----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30514/#review71059 -----------------------------------------------------------
Took a look at the tests as well. src/master/master.cpp <https://reviews.apache.org/r/30514/#comment116594> Having this seems good, want to move the comment? ``` if (shuttingDown.isSome()) { return; // Shutdown is already in progress. } ``` src/master/master.cpp <https://reviews.apache.org/r/30514/#comment116600> The .get() seems a bit hacky to me (as Jie said it's not really const), it seems nice to set `shuttingDown` once at the end of the method to the correct future. The Future<Nothing>(Nothing()) also looks a bit cluttered? This was the motivation for obtaining the 'limited' or 'throttle' future first: ``` Future<Nothing> acquire = Nothing(); if (limiter.isSome()) { acquire = limiter.get()->acquire(); } shuttingDown = acquire.onAny(defer(self(), &Self::_shutdown)); ``` Also, I'm not sure what the comments are adding over what's in the code? Thoughts? src/master/master.cpp <https://reviews.apache.org/r/30514/#comment116601> Does this comment add any insight? It looks like the LOG(INFO) line reveals this already :) Also, it looks like you're sometimes using two l's and sometimes using one l :) src/tests/slave_tests.cpp <https://reviews.apache.org/r/30514/#comment116605> Looks good. Do you think we want to inject a rate limiter at some point? - Ben Mahler On Feb. 4, 2015, 10:43 p.m., Vinod Kone wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/30514/ > ----------------------------------------------------------- > > (Updated Feb. 4, 2015, 10:43 p.m.) > > > Review request for mesos, Ben Mahler, David Robinson, and Jie Yu. > > > Bugs: MESOS-1148 > https://issues.apache.org/jira/browse/MESOS-1148 > > > Repository: mesos > > > Description > ------- > > The algorithm is simple. All the slave observers share a rate limiter whose > rate is configured via command line. When a slave times out on health check, > a permit is acquired to shutdown the slave *but* the pings are continued to > be sent. If a pong arrives before the permit is satisifed, the shutdown is > cancelled. > > > Diffs > ----- > > src/master/flags.hpp 6c18a1af625311ef149b5877b08f63c2b12c040d > src/master/master.hpp cd37ee9d3c57bcd91f08cd402ec1e4a09d9e42ee > src/master/master.cpp 69b945df58c173322b95aeae9cf4ed70329c4bb3 > src/tests/partition_tests.cpp fea78016268b007590516798eb30ff423fd0ae58 > src/tests/slave_tests.cpp e7e2af63da785644f3f7e6e23607c02be962a2c6 > > Diff: https://reviews.apache.org/r/30514/diff/ > > > Testing > ------- > > make check > > Ran the new tests 100 times > > > Thanks, > > Vinod Kone > >
