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 72824: Eliminated redundant update broadcasts in UPDATE_FRAMEWORK.
Date Wed, 02 Sep 2020 17:54:20 GMT


> On Aug. 31, 2020, 9:14 p.m., Benjamin Mahler wrote:
> > src/master/master.cpp
> > Line 3291 (original), 3301 (patched)
> > <https://reviews.apache.org/r/72824/diff/1/?file=2238955#file2238955line3301>
> >
> >     The PID could have changed, we can probably just remove that PID code within
`sendFrameworkUpdates` at this point.
> >     
> >     With the PID code still there, one can see that this if condition is not capturing
the potential PID change we'd want to tell the agent about.
> 
> Andrei Sekretenko wrote:
>     Hmm... I'm afraid I do not understand your point.
>     
>     First: can we really stop sending V0 framwork PID updates to agents? 
>     The comment around the framework pid in the messages sent by master to an agent doesn't
say anything about the PID of the V0 frameworks, does it?
>     Also, it looks like the agent code is using the framework PID for sending executor
messages to the V0 frameworks bypassing the master.
>     Although removing the PID update in `sendFrameworkUpdates()` doesn't break any tests,
a naive attempt to remove the framework PID from the agent does.
>     I haven't dived deep into that, but it looks like some tests depend on that bypass;
my suspicion is that this V0 PID update is just not covered by tests.
>     
>     Second: why are you asking about the potential PID change in the `UpdateFramework`
call? 
>     I'm looking at `Master::receive()` https://github.com/apache/mesos/blob/a16f3439dca13982bb4a2b9190c24aaf4eb73b0e/src/master/master.cpp#L2386
and not getting how a V0 framework could change its PID without resubscribing...

Ah.. I see now from your comment and explanation, the PID cannot be changed in this path.
I also forgot that we do still use the PID based master bypass if the scheduler is v0.

I guess eliminating the TODO is just the following:

```
    // TODO(anand): We set 'pid' to UPID() for http frameworks
    // as 'pid' was made optional in 0.24.0. In 0.25.0, we
    // no longer have to set pid here for http frameworks.
    message.set_pid(framework.pid().getOrElse(UPID()));
    message.mutable_framework_info()->CopyFrom(framework.info);
    send(slave->pid, message);
```

vs

```
    *message.mutable_framework_info() = framework.info;
    
    // Agents need to know about v0 framework's PIDs in order to
    // bypass the master for executor to framework messages.
    if (framework.pid().isSome()) {
      message.set_pid(*framework.pid());
    }

    send(slave->pid, message);
```


- Benjamin


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


On Sept. 2, 2020, 2:08 p.m., Andrei Sekretenko wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72824/
> -----------------------------------------------------------
> 
> (Updated Sept. 2, 2020, 2:08 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-10166
>     https://issues.apache.org/jira/browse/MESOS-10166
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This patch makes the UPDATE_FRAMEWORK call avoid sending the
> `UpdatedFrameworkMessage` to agents and the `FRAMEWORK_UPDATED`
> event to V1 API subscribers when the FrameworkInfo is not changed
> by the call.
> 
> 
> Diffs
> -----
> 
>   src/master/master.cpp 02723296e569fac9d553b1494a5ca7daa6ef9aa4 
> 
> 
> Diff: https://reviews.apache.org/r/72824/diff/4/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>


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