Please review ticket 47590 (take #2): add/split functions around replication

[thierry bordaz]

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...

[Roberto Polli]

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")

2-  The method "enableReplication" should probably go into the Replica class, 
and be called like
   conn.replica.enable()

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?

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!

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

[thierry bordaz]

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
>