----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36518/#review91821 -----------------------------------------------------------
Ship it! Looks good given the current Call API. However, per our chat, 'force' doesn't make sense for initial SUBSCRIBE calls, I left a comment for how we might want to address that. =/ src/master/master.cpp (line 1747) <https://reviews.apache.org/r/36518/#comment145459> Seems like we might want to keep the condition consistent across all of these checks (i.e. has_id && id != ""), up to you. At least, would be nice to add an != operator for FrameworkID vs string. src/tests/scheduler_tests.cpp (lines 110 - 112) <https://reviews.apache.org/r/36518/#comment145461> Since there's no RESUBSCRIBE, shall we simply call this test 'Subscribe' (I noticed there is no Subscribe test currently) and test the full semantics within it? Looks like a test of the 'Subscribe' call to me. src/tests/scheduler_tests.cpp (line 143) <https://reviews.apache.org/r/36518/#comment145465> Hm.. 'force' doesn't make sense for SUBSCRIBE without a framework id. Seems like either we: (1) Make 'force' optional, and document that it is only relevant when the framework id is set (re-subscription). (2) Remove 'force' from SUBSCRIBE, add a RESUBSCRIBE with 'force'? =/ src/tests/scheduler_tests.cpp (line 163) <https://reviews.apache.org/r/36518/#comment145462> why the newline here but not above? - Ben Mahler On July 15, 2015, 6:40 p.m., Vinod Kone wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/36518/ > ----------------------------------------------------------- > > (Updated July 15, 2015, 6:40 p.m.) > > > Review request for mesos and Ben Mahler. > > > Bugs: MESOS-3055 > https://issues.apache.org/jira/browse/MESOS-3055 > > > Repository: mesos > > > Description > ------- > > In the process of fixing this, added an additional check in > Master::registerFramework() that should've been there in the first place. > Similar check exists in Master::reregisterFramework(). > > > Diffs > ----- > > src/master/master.cpp b877676afa6f3833eb7d2fb06beeaa288bd8bd5d > src/tests/scheduler_tests.cpp 946fa8245d8ab35e04bad642d69114caf0ccf6a9 > > Diff: https://reviews.apache.org/r/36518/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Vinod Kone > >
