mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Mahler <bmah...@apache.org>
Subject Re: Review Request 72095: Introduced dedicated `Framework` methods for transitions between states.
Date Thu, 27 Feb 2020 22:40:16 GMT

-----------------------------------------------------------
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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message