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 72838: Moved framework update out of `connectAndActivateRecoveredFramework`.
Date Tue, 08 Sep 2020 22:40:16 GMT

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


Ship it!





src/master/master.cpp
Lines 3130-3137 (original), 3120-3127 (patched)
<https://reviews.apache.org/r/72838/#comment310906>

    Not really related to this change, but I got pretty confused here thinking about 'force'.
    
    One thing I was trying to figure out was: if a scheduler failed over, and the old scheduler
instance is disconnected, I would expect that you don't need 'force == true' for the new instance
to connect. But it looks like this check would disallow the new scheduler instance and always
requires `force == true` for a new scheduler instance even if the old one disconnected already.
    
    Reading the comment on `force` in the v0 protobuf, I now realize that it's only used by
the driver and the driver always sets it to true whenever it registers after failover.
    
    For v1 http, looks like it will always wind up as `force == false` during the devolve
call, but we don't even look at `force` in that `_subscribe` path.
    
    Some suggestions:
    
    (1) Looks like this:
    
    // Using the "force" field of the scheduler allows us to keep a
    
    Should be s/keep/reject/ ? That's the case we want to disallow from coming back.
    
    (2) Perhaps we clarify here that the v0 driver always sets `force` to true when it registers
for the first time post failover? Otherwise it leaves it false to ensure the case described
is rejected.



src/master/master.cpp
Line 3137 (original), 3127 (patched)
<https://reviews.apache.org/r/72838/#comment310905>

    since you're touching this line anyway, feel free to remove the unnecessary parens


- Benjamin Mahler


On Sept. 7, 2020, 3:24 p.m., Andrei Sekretenko wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72838/
> -----------------------------------------------------------
> 
> (Updated Sept. 7, 2020, 3:24 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-10179
>     https://issues.apache.org/jira/browse/MESOS-10179
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This patch consolidates updating framework in the Subscribe call
> by moving `updateFramework()` invocation from
> `connectAndActivateRecoveredFramework()` into `Master::_subscribe()`.
> 
> 
> Diffs
> -----
> 
>   src/master/master.hpp 3d1d4723d1bcef1880404007410bf00656399f10 
>   src/master/master.cpp 5b5d5c359ca48eb0b4f5554fe7c91e928d4da08d 
> 
> 
> Diff: https://reviews.apache.org/r/72838/diff/1/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>


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