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

[Martin Schwenke]

On Wed, 9 Apr 2014 17:57:12 +1000, Amitay Isaacs <amitay at gmail.com>
wrote:

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

Given the absence of further comments, I've marked these as reviewed by
me and pushed them to autobuild.

Michael, I wasn't sure if I should mark any of the patches as reviewed
by you, so I decided to be conservative and not speak for you...

peace & happiness,
martin