Dinesh Bhat has posted comments on this change. Change subject: KUDU-1741: Keep MiniCluster::Restart consistent with ExternalMiniCluster::Restart ......................................................................
Patch Set 2: > > Personally I think this patch is fine as-is, because the > particular > > semantics matter a bit less than just being consistent IMHO. If > we > > wanted to change the semantics to work the other way around, we > > could do it, but it would be more work to get right since EMC > gets > > way more use than MC. > > Agreed with Mike; consistency takes precedence here. > > I took a quick look at the various users of Restart() today (both > EMC and the individual daemons), and it's important to note that > quite a few tests do something like this: > > <call Shutdown() to shut something down> > <do some work> > <call Restart() to bring it back up> > > Thus, if we took this in the other direction and made Restart() > also shut things down, we wouldn't be able to handle the above use > case. As another data point, linked_list-test uses ShutdownNodes() > to bring down just the tservers, then Restart() to bring them back > up. > > Personally, I find the proliferation of Start/Stop methods to be > confusing when writing new tests, so my vote would be for 1) > convert Restart() into stop-then-start semantics as JD suggested, > 2) beef up Start() such that it's idempotent and will bring up all > downed servers, and 3) ensure these semantics are consistent not > just across EMC/MC but also on the individual daemons themselves > (i.e. Start() should be idempotent everywhere). > > With the above changes, the back-to-back cluster Shutdown()+Restart() > callers become single calls to Restart(), and the more interesting > cases become <shutdown some stuff>+<do some work>+Start(). > > But, I'm also fine with deferring this (more extensive) change to > later, and to focus on bringing in consistency now. Thanks Adar, I will track these discussions/ideas in a new JIRA for future improvements. -- To view, visit http://gerrit.cloudera.org:8080/5598 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iad33b7c46bfca3f277ccbca7d0420272f06a6633 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dinesh Bhat <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Dinesh Bhat <[email protected]> Gerrit-Reviewer: Jean-Daniel Cryans <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-HasComments: No
