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...
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. 2- methods like _createDefaultReplMgr are not expected to be used in production, so should be placed in the DSAdminTools section 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.
= 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
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
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
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
389-devel@lists.fedoraproject.org
-
Roberto Polli -
thierry bordaz