----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25866/#review54384 -----------------------------------------------------------
Ship it! Phew that was a bit tricky to review because of the renaming + metrics changes sneaking in here. Split it next time to help me out? :) src/master/master.hpp <https://reviews.apache.org/r/25866/#comment94520> Hm.. seems a bit odd to describe the false case explicitly. How about we phrase this a bit differently? // Slaves become disconnected when the socket closes. // No offers are sent for inactive slaves. Currently, ... Does it make sense to comment these together, so that we can explain that disconnected slaves are always inactive? Ditto below for Framework. src/master/master.cpp <https://reviews.apache.org/r/25866/#comment94617> Why do we check slave->connected before calling disconnect below, but we don't check framework->connected here? Probably warrants a comment. src/master/master.cpp <https://reviews.apache.org/r/25866/#comment94621> This can't be moved down to be alongside the framework->active check? Ditto below. src/master/master.cpp <https://reviews.apache.org/r/25866/#comment94615> Should this be below deactivateFramework, to keep disconnect and deactivate adjacent (like in the header file)? src/master/master.cpp <https://reviews.apache.org/r/25866/#comment94619> Re: question above Should this be idempotent? i.e. do nothing when already disconnected. Or do you want the caller to never call this when already disconnected? src/master/master.cpp <https://reviews.apache.org/r/25866/#comment94623> What about saying that we don't expect a slave to be connected and deactivated? Ditto below. src/master/master.cpp <https://reviews.apache.org/r/25866/#comment94620> Looks like connected = true doesn't need to be done before removing offers, correct? - Ben Mahler On Sept. 22, 2014, 11:50 p.m., Vinod Kone wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/25866/ > ----------------------------------------------------------- > > (Updated Sept. 22, 2014, 11:50 p.m.) > > > Review request for mesos, Adam B and Ben Mahler. > > > Bugs: MESOS-1081 and MESOS-1811 > https://issues.apache.org/jira/browse/MESOS-1081 > https://issues.apache.org/jira/browse/MESOS-1811 > > > Repository: mesos-git > > > Description > ------- > > Made consistent what connected and active frameworks/slaves means. > > Fixed MESOS-1811 along the way. > > > Diffs > ----- > > src/master/http.cpp 3f5a01dfddca9cea73563100d88e0c03f600d6b1 > src/master/master.hpp f5d74aef185fad861139186be1cab089f8005a94 > src/master/master.cpp e5d30e9c7ba1ec0cdd640c81610790f3397f3062 > src/tests/fault_tolerance_tests.cpp > 154386044d0247b39d84719d7ff14250682a0695 > src/tests/master_tests.cpp 8e4ec1d85c4530b5421387de55036f7d40ee3180 > > Diff: https://reviews.apache.org/r/25866/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Vinod Kone > >
