You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Dmitry Zhuk <dz...@twopensource.com> on 2017/11/07 16:51:26 UTC
Review Request 63631: Separated event visiting and consumption.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63631/
-----------------------------------------------------------
Review request for mesos, Benjamin Mahler and Michael Park.
Bugs: MESOS-6972
https://issues.apache.org/jira/browse/MESOS-6972
Repository: mesos
Description
-------
This introduces `EventConsumer` interface, which add support of events
with movable only data. This allows consumers to move data out of
event, rather than copying it. This is required to implement movable
only objects support in `defer` to guarantee that deferred functor is
invoked only once, allowing deferred parameters to be moved into call.
Diffs
-----
3rdparty/libprocess/include/process/event.hpp 2e246205020c3c5b8c2eb5187a8eb3643d1e6d4d
3rdparty/libprocess/include/process/process.hpp dc3375ce62556322eb2bc60ade61f313ade123b8
3rdparty/libprocess/include/process/protobuf.hpp fe152f273332470ac50f9715291897bb04cf95b9
3rdparty/libprocess/include/process/run.hpp 1540a78e52a90d4c1d4165c46be353caaad21bce
3rdparty/libprocess/src/process.cpp 71ae7129ffbd0e22eda2863b17bbcf588298c37b
3rdparty/libprocess/src/tests/benchmarks.cpp e6c77d565d5acf72b475a085e9504679253b4b97
3rdparty/libprocess/src/tests/process_tests.cpp 952c92c033e2363cff0c2c68610d3820b97d177e
Diff: https://reviews.apache.org/r/63631/diff/1/
Testing
-------
Thanks,
Dmitry Zhuk
Re: Review Request 63631: Separated event visiting and consumption.
Posted by Michael Park <mp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63631/#review192879
-----------------------------------------------------------
Fix it, then Ship it!
3rdparty/libprocess/include/process/event.hpp
Lines 91-127 (original), 103-144 (patched)
<https://reviews.apache.org/r/63631/#comment271214>
I noticed that we have 2 types of messages:
(1) copyable but not assignable
(2) not copyable nor assignable
I think we should make all of the message properly move-only. (e.g., the changes we made to `HttpEvent`).
For most of them we just need to remove the `const` on the members.
3rdparty/libprocess/include/process/event.hpp
Lines 114-117 (original), 126-129 (patched)
<https://reviews.apache.org/r/63631/#comment271209>
This comment needs to be updated.
3rdparty/libprocess/include/process/event.hpp
Lines 155-158 (original), 178-181 (patched)
<https://reviews.apache.org/r/63631/#comment271210>
Let's clean these up and pull them up and declare them `= delete;`.
Here and below.
- Michael Park
On Dec. 4, 2017, 8:01 a.m., Dmitry Zhuk wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63631/
> -----------------------------------------------------------
>
> (Updated Dec. 4, 2017, 8:01 a.m.)
>
>
> Review request for mesos, Benjamin Mahler and Michael Park.
>
>
> Bugs: MESOS-6972
> https://issues.apache.org/jira/browse/MESOS-6972
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This introduces `EventConsumer` interface, which add support of events
> with movable only data. This allows consumers to move data out of
> event, rather than copying it. This is required to implement movable
> only objects support in `defer` to guarantee that deferred functor is
> invoked only once, allowing deferred parameters to be moved into call.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/event.hpp 571ffb1ea5c4787f10c2f4dee249aadc8b904bc6
> 3rdparty/libprocess/include/process/process.hpp d88c2f241a04cac3ec2e1d9fd4b323643ad24ae4
> 3rdparty/libprocess/include/process/protobuf.hpp 08605db4aa7ae782f1e5a1f692db9d46f899f842
> 3rdparty/libprocess/include/process/run.hpp 46a182b2dd4ab7add6d6f547e83958b19fa2a0f2
> 3rdparty/libprocess/src/process.cpp 64bcce215d19558dd493e30e96ca16577fe0722a
> 3rdparty/libprocess/src/tests/benchmarks.cpp 2c6de626c3fdf5ebd3a7aa5793553215c42dd494
> 3rdparty/libprocess/src/tests/process_tests.cpp d1b30b0754c55fd765885ddcfe9f9f68ba509ac1
>
>
> Diff: https://reviews.apache.org/r/63631/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Dmitry Zhuk
>
>
Re: Review Request 63631: Separated event visiting and consumption.
Posted by Dmitry Zhuk <dz...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63631/
-----------------------------------------------------------
(Updated Dec. 5, 2017, 6 p.m.)
Review request for mesos, Benjamin Mahler and Michael Park.
Bugs: MESOS-6972
https://issues.apache.org/jira/browse/MESOS-6972
Repository: mesos
Description
-------
This introduces `EventConsumer` interface, which add support of events
with movable only data. This allows consumers to move data out of
event, rather than copying it. This is required to implement movable
only objects support in `defer` to guarantee that deferred functor is
invoked only once, allowing deferred parameters to be moved into call.
Diffs (updated)
-----
3rdparty/libprocess/include/process/event.hpp 571ffb1ea5c4787f10c2f4dee249aadc8b904bc6
3rdparty/libprocess/include/process/process.hpp d88c2f241a04cac3ec2e1d9fd4b323643ad24ae4
3rdparty/libprocess/include/process/protobuf.hpp 08605db4aa7ae782f1e5a1f692db9d46f899f842
3rdparty/libprocess/include/process/run.hpp 46a182b2dd4ab7add6d6f547e83958b19fa2a0f2
3rdparty/libprocess/src/process.cpp 64bcce215d19558dd493e30e96ca16577fe0722a
3rdparty/libprocess/src/tests/benchmarks.cpp 2c6de626c3fdf5ebd3a7aa5793553215c42dd494
3rdparty/libprocess/src/tests/process_tests.cpp d1b30b0754c55fd765885ddcfe9f9f68ba509ac1
Diff: https://reviews.apache.org/r/63631/diff/3/
Changes: https://reviews.apache.org/r/63631/diff/2-3/
Testing
-------
Thanks,
Dmitry Zhuk
Re: Review Request 63631: Separated event visiting and consumption.
Posted by Dmitry Zhuk <dz...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63631/
-----------------------------------------------------------
(Updated Dec. 4, 2017, 4:01 p.m.)
Review request for mesos, Benjamin Mahler and Michael Park.
Changes
-------
Addressed review comments.
Bugs: MESOS-6972
https://issues.apache.org/jira/browse/MESOS-6972
Repository: mesos
Description
-------
This introduces `EventConsumer` interface, which add support of events
with movable only data. This allows consumers to move data out of
event, rather than copying it. This is required to implement movable
only objects support in `defer` to guarantee that deferred functor is
invoked only once, allowing deferred parameters to be moved into call.
Diffs (updated)
-----
3rdparty/libprocess/include/process/event.hpp 571ffb1ea5c4787f10c2f4dee249aadc8b904bc6
3rdparty/libprocess/include/process/process.hpp d88c2f241a04cac3ec2e1d9fd4b323643ad24ae4
3rdparty/libprocess/include/process/protobuf.hpp 08605db4aa7ae782f1e5a1f692db9d46f899f842
3rdparty/libprocess/include/process/run.hpp 46a182b2dd4ab7add6d6f547e83958b19fa2a0f2
3rdparty/libprocess/src/process.cpp 64bcce215d19558dd493e30e96ca16577fe0722a
3rdparty/libprocess/src/tests/benchmarks.cpp 2c6de626c3fdf5ebd3a7aa5793553215c42dd494
3rdparty/libprocess/src/tests/process_tests.cpp d1b30b0754c55fd765885ddcfe9f9f68ba509ac1
Diff: https://reviews.apache.org/r/63631/diff/2/
Changes: https://reviews.apache.org/r/63631/diff/1-2/
Testing
-------
Thanks,
Dmitry Zhuk
Re: Review Request 63631: Separated event visiting and consumption.
Posted by Michael Park <mp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63631/#review192512
-----------------------------------------------------------
Fix it, then Ship it!
The `ProcessBase` and `EventVisitor` relationship is a bit weird already,
and I'm a bit concerned about essentially changing the definition of
`ProcessBase` across the entire codebase, but I think overall the patch
looks consistent and correct.
3rdparty/libprocess/src/tests/benchmarks.cpp
Lines 608-609 (original), 608-609 (patched)
<https://reviews.apache.org/r/63631/#comment270702>
Let's remove this since we're constructing new ones within the `for` loop now.
- Michael Park
On Nov. 7, 2017, 8:51 a.m., Dmitry Zhuk wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63631/
> -----------------------------------------------------------
>
> (Updated Nov. 7, 2017, 8:51 a.m.)
>
>
> Review request for mesos, Benjamin Mahler and Michael Park.
>
>
> Bugs: MESOS-6972
> https://issues.apache.org/jira/browse/MESOS-6972
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This introduces `EventConsumer` interface, which add support of events
> with movable only data. This allows consumers to move data out of
> event, rather than copying it. This is required to implement movable
> only objects support in `defer` to guarantee that deferred functor is
> invoked only once, allowing deferred parameters to be moved into call.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/event.hpp 2e246205020c3c5b8c2eb5187a8eb3643d1e6d4d
> 3rdparty/libprocess/include/process/process.hpp dc3375ce62556322eb2bc60ade61f313ade123b8
> 3rdparty/libprocess/include/process/protobuf.hpp fe152f273332470ac50f9715291897bb04cf95b9
> 3rdparty/libprocess/include/process/run.hpp 1540a78e52a90d4c1d4165c46be353caaad21bce
> 3rdparty/libprocess/src/process.cpp 71ae7129ffbd0e22eda2863b17bbcf588298c37b
> 3rdparty/libprocess/src/tests/benchmarks.cpp e6c77d565d5acf72b475a085e9504679253b4b97
> 3rdparty/libprocess/src/tests/process_tests.cpp 952c92c033e2363cff0c2c68610d3820b97d177e
>
>
> Diff: https://reviews.apache.org/r/63631/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Dmitry Zhuk
>
>
Re: Review Request 63631: Separated event visiting and consumption.
Posted by Michael Park <mp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63631/#review192686
-----------------------------------------------------------
3rdparty/libprocess/include/process/event.hpp
Lines 172 (patched)
<https://reviews.apache.org/r/63631/#comment270934>
It looks like `consume` in this case happens to not actually move the `HttpEvent`, but if it were to we'd be in trouble due to the fact that `HttpEvent` holds a raw pointer and the move ctor is not defined accordingly.
- Michael Park
On Nov. 7, 2017, 8:51 a.m., Dmitry Zhuk wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63631/
> -----------------------------------------------------------
>
> (Updated Nov. 7, 2017, 8:51 a.m.)
>
>
> Review request for mesos, Benjamin Mahler and Michael Park.
>
>
> Bugs: MESOS-6972
> https://issues.apache.org/jira/browse/MESOS-6972
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This introduces `EventConsumer` interface, which add support of events
> with movable only data. This allows consumers to move data out of
> event, rather than copying it. This is required to implement movable
> only objects support in `defer` to guarantee that deferred functor is
> invoked only once, allowing deferred parameters to be moved into call.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/event.hpp 2e246205020c3c5b8c2eb5187a8eb3643d1e6d4d
> 3rdparty/libprocess/include/process/process.hpp dc3375ce62556322eb2bc60ade61f313ade123b8
> 3rdparty/libprocess/include/process/protobuf.hpp fe152f273332470ac50f9715291897bb04cf95b9
> 3rdparty/libprocess/include/process/run.hpp 1540a78e52a90d4c1d4165c46be353caaad21bce
> 3rdparty/libprocess/src/process.cpp 71ae7129ffbd0e22eda2863b17bbcf588298c37b
> 3rdparty/libprocess/src/tests/benchmarks.cpp e6c77d565d5acf72b475a085e9504679253b4b97
> 3rdparty/libprocess/src/tests/process_tests.cpp 952c92c033e2363cff0c2c68610d3820b97d177e
>
>
> Diff: https://reviews.apache.org/r/63631/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Dmitry Zhuk
>
>