On 11/18/2013 11:33 AM, Roberto Polli wrote:
On Monday 18 November 2013 11:08:01 thierry bordaz wrote:
Hi, this is a second review to take into account the recommendation of
the first review.
Major changes are:
* Create a replicaagreement class in brooker
* mv init/status/schedule/create in that new class
* mv createDefaultReplMgr into the brooker replica class
* Handling of error condition with exceptions
https://fedorahosted.org/389/attachment/ticket/47590/0002-Ticket-47590-CI-te
sts-add-split-functions-around-rep.patch
1- Here and elsewhere:
if not suffix:
# This is a mandatory parameter of the command... it fails
log.warning("createAgreement: suffix is missing")
- return None
+ raise ValueError("suffix is a required value")
Excellent. Thanks for the tips
2- The method "enableReplication" should probably go into the Replica class,
and be called like
conn.replica.enable()
Yes I have additional cleanup to do moving things from lib389 ->
agreement or replica (like createAgreement or enableReplication) or
replica->agreement.... I wanted in that ticket to split big functions in
more specific/atomic functions before implementing a replication test case.
I will open an other ticket to do that cleanup
3- enableReplication and agreementInit should raise in case of errors, not
return 1 or -1. Managing error codes is extra work: and moreover which value
is fine? Greater than zero? Zero? True?
ok
4- If you put replica_createReplMgr in the Replica brooker, just name it
create_manager() so that we can call it
conn.replica.create_manager(). The replica is inferred from the context!
ok
5- If you move the agreement stuff outside Replica, I would just name the
class "Agreement" and set
self.agreement = Agreement(self)
or
self.replica_agreement
ok
--
389-devel mailing list
389-devel@xxxxxxxxxxxxxxxxxxxxxxx
https://admin.fedoraproject.org/mailman/listinfo/389-devel
