You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Andrei Sekretenko <as...@mesosphere.io> on 2020/02/07 17:32:15 UTC

Review Request 72095: Tied tracking framework state and connection state together.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72095/
-----------------------------------------------------------

Review request for mesos, Benjamin Mahler and Greg Mann.


Bugs: MESOS-10056
    https://issues.apache.org/jira/browse/MESOS-10056


Repository: mesos


Description
-------

Tied tracking framework state and connection state together.


Diffs
-----

  src/master/framework.cpp e69a7c26d15ffffb3d147328032f996962387c96 
  src/master/master.hpp c813e9fc855cfb1701ec32be7f690e06b6eb203f 
  src/master/master.cpp d41ae724ba12b5ad1c8ae3c1f9b91a05b0e46e7e 


Diff: https://reviews.apache.org/r/72095/diff/1/


Testing
-------


Thanks,

Andrei Sekretenko

Re: Review Request 72095: Tied tracking framework state and connection state together.

Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72095/#review219526
-----------------------------------------------------------



I'm having trouble understanding the purpose or the overall thinking behind this change, can you explain in the description to guide the reader?

- Benjamin Mahler


On Feb. 7, 2020, 5:32 p.m., Andrei Sekretenko wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72095/
> -----------------------------------------------------------
> 
> (Updated Feb. 7, 2020, 5:32 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
> -------
> 
> Tied tracking framework state and connection state together.
> 
> 
> Diffs
> -----
> 
>   src/master/framework.cpp e69a7c26d15ffffb3d147328032f996962387c96 
>   src/master/master.hpp c813e9fc855cfb1701ec32be7f690e06b6eb203f 
>   src/master/master.cpp d41ae724ba12b5ad1c8ae3c1f9b91a05b0e46e7e 
> 
> 
> Diff: https://reviews.apache.org/r/72095/diff/1/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>

Re: Review Request 72095: Introduced dedicated `Framework` methods for transitions between states.

Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
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
> 
>

Re: Review Request 72095: Introduced dedicated `Framework` methods for transitions between states.

Posted by Andrei Sekretenko <as...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72095/
-----------------------------------------------------------

(Updated Feb. 28, 2020, 5:37 p.m.)


Review request for mesos, Benjamin Mahler and Greg Mann.


Changes
-------

Changed `isActive()` back to `active`, addressed other issues.


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 (updated)
-----

  src/master/framework.cpp a9318a9d33122610960e01a184b568a8ea18b514 
  src/master/master.hpp d774d77a50597770c6f2d4f5dffcbd79b5f29da3 
  src/master/master.cpp 36a81ccd24d0156049382fee0d085193cc2867e6 
  src/master/quota_handler.cpp ea3f85887c96e0a0b14bcb2eb33646032868e0c8 


Diff: https://reviews.apache.org/r/72095/diff/5/

Changes: https://reviews.apache.org/r/72095/diff/4-5/


Testing
-------


Thanks,

Andrei Sekretenko

Re: Review Request 72095: Introduced dedicated `Framework` methods for transitions between states.

Posted by Andrei Sekretenko <as...@mesosphere.io>.
-----------------------------------------------------------
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.


Changes
-------

Decoupled RECOVERED/CONNECTED/DISCONNECTED from active/inactive.

Also fixed minor issues.


Bugs: MESOS-10056
    https://issues.apache.org/jira/browse/MESOS-10056


Repository: mesos


Description (updated)
-------

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 (updated)
-----

  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/

Changes: https://reviews.apache.org/r/72095/diff/3-4/


Testing
-------


Thanks,

Andrei Sekretenko

Re: Review Request 72095: Introduced dedicated `Framework` methods for transitions between states.

Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
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
> 
>

Re: Review Request 72095: Introduced dedicated `Framework` methods for transitions between states.

Posted by Andrei Sekretenko <as...@mesosphere.io>.
-----------------------------------------------------------
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.


Changes
-------

Adjusted for changes in previous patch; expanded description.


Summary (updated)
-----------------

Introduced dedicated `Framework` methods for transitions between states.


Bugs: MESOS-10056
    https://issues.apache.org/jira/browse/MESOS-10056


Repository: mesos


Description (updated)
-------

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 (updated)
-----

  src/master/framework.cpp e69a7c26d15ffffb3d147328032f996962387c96 
  src/master/master.hpp c813e9fc855cfb1701ec32be7f690e06b6eb203f 
  src/master/master.cpp d41ae724ba12b5ad1c8ae3c1f9b91a05b0e46e7e 


Diff: https://reviews.apache.org/r/72095/diff/2/

Changes: https://reviews.apache.org/r/72095/diff/1-2/


Testing
-------


Thanks,

Andrei Sekretenko