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 65971: Avoid a copy of the scheduler->executor message in the master.
Date Fri, 09 Mar 2018 20:34:53 GMT


> On March 9, 2018, 8:01 p.m., Meng Zhu wrote:
> > src/master/master.cpp
> > Lines 5902-5905 (original), 5902-5905 (patched)
> > <https://reviews.apache.org/r/65971/diff/1/?file=1972463#file1972463line5902>
> >
> >     Also move this?

This was moved in https://reviews.apache.org/r/65972/


> On March 9, 2018, 8:01 p.m., Meng Zhu wrote:
> > src/master/master.cpp
> > Line 5907 (original), 5907 (patched)
> > <https://reviews.apache.org/r/65971/diff/1/?file=1972463#file1972463line5907>
> >
> >     I found this call chain really confusing.
> >     This function handles `FrameworkToExecutorMessage` by converting it into `scheduler::Call::Message`
and pass it to `message()`. In `message()`, we just convert it back to `FrameworkToExecutorMessage`
and send it. I wonder why don't we just send `FrameworkToExecutorMessage` straight away?
> >     
> >     This seems to adhere to the pattern of converting internal message (used by
v0) into Call and then process the call and generate other internal messages as needed. The
confusing part here is that the message we receive happens to be the same to the message we
send.
> >     
> >     Do you think it is better to just forward it here instead of going through the
unnecessary conversion?
> >     Or add some comment regarding the detour?

The pattern for v0 messages is to convert to v1 message and go through the v1 path, so we
should stick to that for consistency. My hope is that with moves as opposed to copies the
overhead of this transformation becomes negligable. I'm not sure if adding a comment here
to describe the pattern is a good idea because that comment would need to be in every v0 handler,
my hope is that someone reading the code would soon afterwards discover the v0 pattern (much
like you did in your comment). We could consider a comment at the `install`s within `Master::initialize()`
to describe the pattern but would you have read that code?


- Benjamin


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


On March 8, 2018, 4:30 a.m., Benjamin Mahler wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65971/
> -----------------------------------------------------------
> 
> (Updated March 8, 2018, 4:30 a.m.)
> 
> 
> Review request for mesos and Meng Zhu.
> 
> 
> Bugs: MESOS-8628
>     https://issues.apache.org/jira/browse/MESOS-8628
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This avoids a copy in both the v1 http and v0 message code paths.
> 
> 
> Diffs
> -----
> 
>   src/master/http.cpp acf10a0f396234ec9f444c9b41515d657379d6ad 
>   src/master/master.hpp c9c8a968b6f56fe261ac9fe374e926a28d40fccb 
>   src/master/master.cpp e666b664dd125317cda5d16285d444b9c21e1f35 
> 
> 
> Diff: https://reviews.apache.org/r/65971/diff/1/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>


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