----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70996/#review216421 -----------------------------------------------------------
src/master/http.cpp Lines 3879-3881 (patched) <https://reviews.apache.org/r/70996/#comment303646> Nit: I think we usually place the `&&` at the end of the preceding line rather than the beginning of the next. Ditto below. Also, I wonder if we should factor this block out into a helper like `bool isKnownSlave(const SlaveID&)`? Then we have a single location to update if/when we add more containers of known slaves? src/master/master.hpp Lines 678 (patched) <https://reviews.apache.org/r/70996/#comment303650> Provide a comment documenting this new function? It may not be consistent, but let's start setting a good example now :) src/master/master.hpp Lines 2109-2110 (patched) <https://reviews.apache.org/r/70996/#comment303647> s/both admitted and unreachable/admitted, unreachable, and recovered/ Ditto below. src/master/master.cpp Lines 3452 (patched) <https://reviews.apache.org/r/70996/#comment303648> Having this conditional within `Master::reactivate()` looks a bit weird to me. What do you think about moving it to the callsite in `Master::___reregisterSlave()`, and instead placing a `CHECK(!slaves.deactivated.contains(slave->id));` within this function? - Greg Mann On July 2, 2019, 7:57 p.m., Joseph Wu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70996/ > ----------------------------------------------------------- > > (Updated July 2, 2019, 7:57 p.m.) > > > Review request for mesos, Benjamin Bannier, Benjamin Mahler, Greg Mann, and > Vinod Kone. > > > Bugs: MESOS-9814 > https://issues.apache.org/jira/browse/MESOS-9814 > > > Repository: mesos > > > Description > ------- > > This fleshes out three master calls: > * DRAIN_AGENT > * DEACTIVATE_AGENT > * REACTIVATE_AGENT > > The master now stores a mapping of agent draining or deactivation > information. When an agent reconnects, this information is checked > before informing the allocator about the agent. > > This commit leaves out certain aspects of each endpoint's validation, > such as checking if draining agents are not in maintenance schedules. > > The DRAIN_AGENT call is not completely implemented here, because the > transition from DRAINING to DRAINED will be added in a separate commit. > > > Diffs > ----- > > src/master/http.cpp b42ebb953e0510e83ec6bd041cbddbeb8f60067c > src/master/master.hpp 23dafe746b6f9b3d70ad7220f54c4d49068b8af8 > src/master/master.cpp 5247377c2e7e92b9843dd4c9d28f92ba679ad742 > > > Diff: https://reviews.apache.org/r/70996/diff/1/ > > > Testing > ------- > > TODO: At this point, there isn't anything exposed by the master which can be > used to check the results of these APIs. > > > Thanks, > > Joseph Wu > >
