You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Benjamin Mahler <bm...@apache.org> on 2018/03/08 04:30:44 UTC
Review Request 65971: Avoid a copy of the scheduler->executor message
in the master.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65971/
-----------------------------------------------------------
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
Re: Review Request 65971: Avoid a copy of the scheduler->executor
message in the master.
Posted by Benjamin Mahler <bm...@apache.org>.
> 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
>
>
Re: Review Request 65971: Avoid a copy of the scheduler->executor
message in the master.
Posted by Meng Zhu <mz...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65971/#review198957
-----------------------------------------------------------
Fix it, then Ship it!
src/master/master.cpp
Lines 5902-5905 (original), 5902-5905 (patched)
<https://reviews.apache.org/r/65971/#comment279236>
Also move this?
src/master/master.cpp
Line 5907 (original), 5907 (patched)
<https://reviews.apache.org/r/65971/#comment279238>
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?
- Meng Zhu
On March 7, 2018, 8:30 p.m., Benjamin Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65971/
> -----------------------------------------------------------
>
> (Updated March 7, 2018, 8:30 p.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
>
>