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, 20 Feb 2020 14:51:03 GMT

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


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