[PATCH] CTDB: add a new command to detach databases

[Amitay Isaacs]

Hi Michael,

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


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


> Cheers - Michael
>
>
Amitay.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ctdb.patches
Type: application/octet-stream
Size: 16716 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20140409/e46cb536/attachment.obj>