----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72095/#review219617 -----------------------------------------------------------
We should try to make it easy to wire up deactivation in the future (in the same way that agent deactivation works), which would require two different bits for connection and activeness (and we probably want to have the code look similar if possible). src/master/master.hpp Lines 2533-2537 (original), 2533-2541 (patched) <https://reviews.apache.org/r/72095/#comment307804> Up to you, personally I would lean towards just naming them without the "try" and having the return type tell me whether it can fail (error) or be a no-op (bool). src/master/master.hpp Lines 2656-2659 (patched) <https://reviews.apache.org/r/72095/#comment307805> Maybe just merge the comment into the TODO to make it clearer what "this" is referring to: // TODO(...): Encapsulate `state` to ensure that `metrics.subscribed` is updated together with any `state` change. src/master/master.cpp Line 3137 (original), 3137 (patched) <https://reviews.apache.org/r/72095/#comment307806> you can use `*` instead of .get() src/master/master.cpp Line 10572 (original), 10569 (patched) <https://reviews.apache.org/r/72095/#comment307808> period at the end src/master/master.cpp Lines 10574-10577 (original), 10571-10574 (patched) <https://reviews.apache.org/r/72095/#comment307807> feel free to change these to use * since we're touching them src/master/master.cpp Line 10582 (original), 10579 (patched) <https://reviews.apache.org/r/72095/#comment307809> seems like this comment isn't adding anything? - Benjamin Mahler On Feb. 12, 2020, 5:56 p.m., Andrei Sekretenko wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72095/ > ----------------------------------------------------------- > > (Updated Feb. 12, 2020, 5:56 p.m.) > > > Review request for mesos, Benjamin Mahler and Greg Mann. > > > Bugs: MESOS-10056 > https://issues.apache.org/jira/browse/MESOS-10056 > > > Repository: mesos > > > Description > ------- > > The main purpose of this patch is gathering scattered logic of > transitioning `Framework` to disconnected state into > `Framework::tryDisconnect()` method. This is a prerequisite for adding > to the `Framework` state one more entity that needs cleanup when the > framework is disconnected (namely, adding per-framework > `ObjectApprovers` in depending patches). > > For consistency between transition into DISCONNECTED and other state > transitions, this patch also gets rid of public `setFrameworkState(...)` > method and introduces `tryDeactivate()` and `reactivate()` methods. > > > Diffs > ----- > > src/master/framework.cpp e69a7c26d15ffffb3d147328032f996962387c96 > src/master/master.hpp c813e9fc855cfb1701ec32be7f690e06b6eb203f > src/master/master.cpp d41ae724ba12b5ad1c8ae3c1f9b91a05b0e46e7e > > > Diff: https://reviews.apache.org/r/72095/diff/3/ > > > Testing > ------- > > > Thanks, > > Andrei Sekretenko > >
