----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34736/#review85604 -----------------------------------------------------------
Looks good. One high level comment is that we may need to _not_ type param this test, after the resource estimator module patch lands. src/master/master.cpp <https://reviews.apache.org/r/34736/#comment137214> Do we use semi-colon in log lines? Have mostly seen ':' Want to add the resources which are now _not_ updated? src/master/master.cpp <https://reviews.apache.org/r/34736/#comment137215> You should be able to do this in one line, right? Slave* slave = CHECK_NOTNULL(slaves.registered.get(slaveId)); src/master/master.cpp <https://reviews.apache.org/r/34736/#comment137208> Should we specify that it is the sum/total (updated + allocated)? src/master/master.cpp <https://reviews.apache.org/r/34736/#comment137219> Did we want to do some math and only rescind when we have to (eventually)? src/master/master.cpp <https://reviews.apache.org/r/34736/#comment137217> Can we const 'offered'? src/master/master.cpp <https://reviews.apache.org/r/34736/#comment137221> End with period and/or move above this line :) src/tests/oversubscription_tests.cpp <https://reviews.apache.org/r/34736/#comment137222> s/opts for/enables/? src/tests/oversubscription_tests.cpp <https://reviews.apache.org/r/34736/#comment137225> Just FYI - after the Resource Estimator patch set. This becomes a typed test and we need to rebase this patch. src/tests/oversubscription_tests.cpp <https://reviews.apache.org/r/34736/#comment137227> I am unclear whether we decided to drop this, as it is implicit or have it be explicit. We should capture it in the style guide, one way or another :) src/tests/oversubscription_tests.cpp <https://reviews.apache.org/r/34736/#comment137228> We may have to think a bit over this; with the resource estimator change (So we only have access to the interface, not a TestResourceEstimator). We won't have the same flexibility. - Niklas Nielsen On May 27, 2015, 4:10 p.m., Vinod Kone wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/34736/ > ----------------------------------------------------------- > > (Updated May 27, 2015, 4:10 p.m.) > > > Review request for mesos, Jie Yu, Joris Van Remoortere, and Niklas Nielsen. > > > Bugs: MESOS-2733 > https://issues.apache.org/jira/browse/MESOS-2733 > > > Repository: mesos > > > Description > ------- > > Rescinds offers and updates the allocator. > > Note that the offers are not split yet. > > > Diffs > ----- > > src/master/master.cpp 1526f59e7c6b135657550eab2ca46216923a01f6 > src/tests/oversubscription_tests.cpp > 75c25b04c1e6a8e0e7e8fd55440743fe1699af88 > > Diff: https://reviews.apache.org/r/34736/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Vinod Kone > >
