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

[thierry bordaz]

On 11/14/2013 12:05 PM, Roberto Polli wrote:
> Hi Thierry,
>
> my consideration follows (a github-like platform with inline comments would be
> really welcome)!
>
> = method naming and placement =
> 1- I would use the following convention: if a method setup a functionality
> adding various entries to the tree, I would name it "setup" and hopefully
> should be placed DSAdminTools.

Hi Roberto,

Sorry for this late feedback.
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' ?

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.
> 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. 
In that case we may offer 'createDefaultReplMgr' (without heading '_').
In your opinion what kind of functions would go into DSAdminTools ? all 
'setupxxx' functions ?
> 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.
>
> = 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