[PATCH] CTDB: add a new command to detach databases
amitay at gmail.com
Wed Apr 9 01:57:12 MDT 2014
On Tue, Apr 8, 2014 at 6:41 PM, Michael Adam <obnox at samba.org> wrote:
> Hi Amitay,
> generally, the patches look very good, just a few comments
> from a first review, without having really tested the new
> functionaltiy yet:
Thanks for the review. I have now added a test for ctdb detach
> 1) [p1] I am not certain that it is desirable to forbid
> detaching when the tunable AllowClientDBAccess == 0.
> I thought a good workflow to have ctdb close its
> local copies might be this:
> - ctdb setvar AllowClientDBAccess 1
> - ctdb detach_db ...
> Otherwise closing a db and forbidding attaching again is racy!
AllowClientDBAccess is per node tunable. So you can only set
AllowClientDBAccess=1 on one of the nodes, do ctdb detach, and then disable
access again. Since you are changing the tunable only on one node, I don't
see how there would be a race condition.
> 2) [p1] In ctdb_control_db_detach(), there are two
> lines with DEBUG messages slighlty exceeding 80 characters,
> also the function header is too long.
> 3) [p1] In ctdb_control_db_detach(), there is an error in a comment:
> "Do the actual detach only if the control comes other daemon."
> does not make complete sense. ("from" missing?)
> 4) [p3] header line of control_detach() is slightly too long,
> as is a debug statement in that function.
> 5) [p4] Maybe
> "disconnect specified database(s) on all nodes in the cluster"
> instead of ^^^
> "disconnect specified database(s) on the nodes in the cluster"
> for clarity?
I have included your suggestions. Please find attached the modified
> Cheers - Michael
-------------- next part --------------
A non-text attachment was scrubbed...
Size: 16716 bytes
Desc: not available
More information about the samba-technical