Adar Dembo has posted comments on this change. Change subject: KUDU-1741: Make MiniCluster and ExternalMiniCluster follow one semantic for Restart ......................................................................
Patch Set 1: > 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. -- 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: 1 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
