----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72095/#review219686 -----------------------------------------------------------
Fix it, then Ship it! src/master/master.cpp Line 12498 (original), 12524 (patched) <https://reviews.apache.org/r/72095/#comment307897> Why rename this? Looks inconsistent with the other functions (e.g. recovered()). We tend to prefer the existing name style in general: active over isActive recovered over isRecovered empty over isEmpty disconnected over isDisconnected etc src/master/framework.cpp Lines 599 (patched) <https://reviews.apache.org/r/72095/#comment307899> This seems a little confusing in terms of the meaning of the return, how about: ``` bool noop = active; active = true; return !noop; ``` Which seems to describe what is being returned very clearly Or: ``` if (active) return false; active = true; return true; ``` which is more consistent with some others e.g. disconnect() src/master/framework.cpp Lines 605-608 (patched) <https://reviews.apache.org/r/72095/#comment307900> Ditto here: ``` bool noop = !active; active = false; return noop; ``` Or: ``` if (!active) return false; active = false; return true; ``` src/master/master.hpp Lines 2531-2533 (original), 2535-2538 (patched) <https://reviews.apache.org/r/72095/#comment307901> Ditto the comment about keeping it as active() for consistency and a smaller diff. I'm realizing now that I see this code and the newline that you likely renamed it to try to signal that activeness is an orthogonal boolean property. I don't think the "is" prefix solves that problem, and maybe a solution (for a later patch) is to expose `state()` rather than `connected()` and `recovered()` to show the difference. src/master/master.cpp Line 9785 (original), 9785 (patched) <https://reviews.apache.org/r/72095/#comment307902> thanks for this attention to detail in accurately logging! maybe it's just me but reads a bit weird without the oxford comma: is not connected, or is not active src/master/master.cpp Lines 9997-9998 (original), 10000-10010 (patched) <https://reviews.apache.org/r/72095/#comment307903> hm.. maybe these two cases could be combined with a ternary expression in the logging statement? src/master/master.cpp Lines 10592-10593 (original), 10602-10603 (patched) <https://reviews.apache.org/r/72095/#comment307904> This probably won't be true in the future since we'd likely track deactivation separately, but looks fine for now src/master/master.cpp Lines 10915-10916 (original), 10926-10927 (patched) <https://reviews.apache.org/r/72095/#comment307905> Seems clear to me without the comment? src/master/quota_handler.cpp Line 250 (original), 250 (patched) <https://reviews.apache.org/r/72095/#comment307898> Seems clearer in the other order: if (framework->connected() && framework->active()) - Benjamin Mahler On Feb. 27, 2020, 6:25 p.m., Andrei Sekretenko wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72095/ > ----------------------------------------------------------- > > (Updated Feb. 27, 2020, 6:25 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::disconnect()` 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). > > Additionally, this patch decouples connection state from eligibility > to receive offers: `ACTIVE` and `INACTIVE` states are merged into > `CONNECTED`, and a new boolean attribute `active` is introduced. > Now that `updateConnection(...)` does not change `active` on its own, > methods `activate()` and `deactivate()` are introduced. > > Note that the current behaviour of activating reconnected framework > regardless of whether it was active before disconnecting is not changed. > > Also, for consistency between `CONNECTED`->`DISCONNECTED` transition > and other state transitions, public `setFrameworkState(...)` method > is removed. > > > Diffs > ----- > > src/common/protobuf_utils.cpp 7fe4a44b1e7ded998dffb0490c1d61ced697ebd5 > src/master/framework.cpp a9318a9d33122610960e01a184b568a8ea18b514 > src/master/http.cpp 67572a3ffa15b6fbc23d2c2a202023ac9b18cdca > src/master/master.hpp d774d77a50597770c6f2d4f5dffcbd79b5f29da3 > src/master/master.cpp 36a81ccd24d0156049382fee0d085193cc2867e6 > src/master/quota_handler.cpp ea3f85887c96e0a0b14bcb2eb33646032868e0c8 > src/master/readonly_handler.cpp f9c000643d64c9e30849d0d56f329ae052ffd137 > > > Diff: https://reviews.apache.org/r/72095/diff/4/ > > > Testing > ------- > > > Thanks, > > Andrei Sekretenko > >
