> On July 15, 2015, 9:38 p.m., Ben Mahler wrote: > > src/tests/scheduler_tests.cpp, line 143 > > <https://reviews.apache.org/r/36518/diff/1/?file=1012851#file1012851line143> > > > > 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'? =/
I'll make 'force' optional in a dependent different review. > On July 15, 2015, 9:38 p.m., Ben Mahler wrote: > > src/tests/scheduler_tests.cpp, line 163 > > <https://reviews.apache.org/r/36518/diff/1/?file=1012851#file1012851line163> > > > > why the newline here but not above? looked dense. will remove. > On July 15, 2015, 9:38 p.m., Ben Mahler wrote: > > src/tests/scheduler_tests.cpp, lines 110-112 > > <https://reviews.apache.org/r/36518/diff/1/?file=1012851#file1012851line110> > > > > 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. done. > On July 15, 2015, 9:38 p.m., Ben Mahler wrote: > > src/master/master.cpp, line 1747 > > <https://reviews.apache.org/r/36518/diff/1/?file=1012850#file1012850line1747> > > > > 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. updated the condition. i'll add a TODO to add != operator and do a sweep. - Vinod ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36518/#review91821 ----------------------------------------------------------- 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 > >
