[389-devel] Please review ticket 47590: add/split functions around replication

[Roberto Polli]

Hi T,

sorry for the laconicism but that's the only way to contribute today.

On Thursday 14 November 2013 22:23:58 thierry bordaz wrote:
> I agree that 'setup' is commonly used to prepare/initialize a
> functionality. Now I would prefer verbs of action/unaction like
> create/delete, enable/disable, set/get/list. With 'setup' verb we may
> create entries, enable functionality, set properties. If we have a
> function setupAgreement (that creates the RA and enables it by default)
> what is the name for the function that delete the agreement
> 'deleteAgreement' ?
As of now we can still use these names, but the essence is: put them in the 
"Tools"  part and we'll find the right way to use them.

> So far DSAdmintools mainly contains offline functions (like start/stop
> instance). I agree we can put setup functions in it. But I wonder if it
> would be interesting to keep all offline functions in a separated file.
Thanks to inheritance, merging two class is free, splitting not. If tomorrow 
we decide to merge Tools and Admin we just have to mix-in DsAdminTool.
http://en.wikipedia.org/wiki/Mixin#In_Python

> > 2- methods like  _createDefaultReplMgr are not expected to be used in
> > production, so should be placed in the DSAdminTools section
> 
> I am not sure. If someone wants to rapidly deploy a replication
> topology, he would be interested to have a default replication manager.
The idea I put under dsadmin is to remove everything that is not essential: 
less is more (so we have even less base code to test).

 People should have:
 * as few methods as possibile;
 * methods should be explicative and deterministic;
 * should be able to remeber them;
 * should be able to find them using self-completion;

One issue we had with the old Rich library is that it put all the methods at 
the root of the class, so whatever you were trying to do you had too many 
choiches. Moreover the rationale of the behavior was unclear  because methods 
tried to solve automatically as many errors or cases they could... to be clear 
they were too intelligent for the unacquainted user.

> In that case we may offer 'createDefaultReplMgr' (without heading '_').
> In your opinion what kind of functions would go into DSAdminTools ? all
> 'setupxxx' functions ?
I would put into DSAdminTools whatever method is not enough general to be used 
in a standard use case. Moreover consider that complex methods - if exposed - 
should be tested with almost all the input/output. Making complex setup 
methods makes it quite impossible.
Eg. it is fine to decide a default replica type, not fine to put user 
credentials. To clarify I'm even against using a default database file name 
for backends.


> > 3- the brooker naming convention is based on the "function-first" so that
> > python interactive users can tab and autocomplete it.
> > 
> >   initAgreement should be renamed to something like agreement_init or
> >   something> 
> > else. For the return codes, simply use exceptions.
> 
> ok. So do you prefer names like 'replica_create', 'suffix_create',
> 'agreement_create', rather than 'createReplica', 'createSuffix',
> 'createAgreement' ? Ok I will change the name.
Right. If you started using ipython and its self-completion stuff you'll 
easily understand the gain of that approach!

It may be worth checking the differences between the original dsadmin code 
used for bugfix and the latest. 

Sorry again for the short time I had to write you this mail :(

Peace,
R.


> > = exception handling & None return =
> > 1- in case of errors, a method should raise a proper exception and
> > eventually log the error
> > 2- so the assert clauses should be replaced by exception because they mean
> > that something went wrong and an action should be taken
> > 3- about the bindmethod stuff: see http://pastebin.com/w0WnVQuJ
> 
> Absolutely, I will change the error handling. In addition, exception
> makes most of the time the code easier to read.
> Thanks
> 
> I will resend a review according to you suggestions.
> 
> regards
> thierry
> 
> > There are some other points but the best way to set them is with patches.
> > 
> > Let me know + Peace,
> > R.
> > 
> > On Wednesday 13 November 2013 17:06:25 thierry bordaz wrote:
> >> In order to implement the first CI test with replication instances, I
> >> reorganised lib389 functions  related to replication setup.
> >> 
> >> https://fedorahosted.org/389/attachment/ticket/47590/0001-Ticket-47590-CI
> >> -te sts-add-split-functions-around-rep.patch

-- 
Roberto Polli
Community Manager
Babel S.r.l. - http://www.babel.it
T: +39.06.9826.9651 M: +39.340.652.2736 F: +39.06.9826.9680
P.zza S.Benedetto da Norcia, 33 - 00040 Pomezia (Roma)

CONFIDENZIALE: Questo messaggio ed i suoi allegati sono di carattere 
confidenziale per i destinatari in indirizzo.
E' vietato l'inoltro non autorizzato a destinatari diversi da quelli indicati 
nel messaggio originale.
Se ricevuto per errore, l'uso del contenuto e' proibito; si prega di 
comunicarlo al mittente e cancellarlo immediatamente.