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 Hindman <be...@berkeley.edu> on 2017/07/22 01:08:24 UTC
Review Request 61052: Refactored Gate and updated Gate per Process
implementation.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61052/
-----------------------------------------------------------
Review request for mesos and Benjamin Mahler.
Repository: mesos
Description
-------
In the original implementation we needed a Gate to potentially outlive
a Process so we stored it in a global map. This patch instead uses
std::shared_ptr to simplfy the code.
Diffs
-----
3rdparty/libprocess/include/process/process.hpp d40179f874754e00b58f271c401650138dc7d01c
3rdparty/libprocess/src/gate.hpp a51610e9b20acfe6cd22ea932efd1e6afad84cf2
3rdparty/libprocess/src/process.cpp b268cdad776a3ca2a87cbe60eb098bde2a70667c
Diff: https://reviews.apache.org/r/61052/diff/1/
Testing
-------
make check
Thanks,
Benjamin Hindman
Re: Review Request 61052: Refactored Gate and updated Gate per Process
implementation.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61052/#review181280
-----------------------------------------------------------
Fix it, then Ship it!
For posterity, could you also add a bit to the description explaining the Gate specific changes? Also, it wasn't clear at first to me whether they were tied into your other changes, and it seems after reading your diff that they can't be pulled apart, so might be worth explaining that in the commit description.
3rdparty/libprocess/src/gate.hpp
Line 21 (original), 23 (patched)
<https://reviews.apache.org/r/61052/#comment256809>
Might be a good time to add a little comment here explaining what a Gate is.
3rdparty/libprocess/src/gate.hpp
Lines 37-38 (original), 26-27 (patched)
<https://reviews.apache.org/r/61052/#comment256805>
This needs to be updated?
3rdparty/libprocess/src/gate.hpp
Lines 37-38 (original), 26-27 (patched)
<https://reviews.apache.org/r/61052/#comment256807>
This needs to be updated?
3rdparty/libprocess/src/gate.hpp
Lines 51-52 (original), 36-37 (patched)
<https://reviews.apache.org/r/61052/#comment256806>
Looks like this needs an update as well, given the removal of a notion of an incrementing 'state'?
3rdparty/libprocess/src/process.cpp
Lines 3361-3363 (original), 3350-3352 (patched)
<https://reviews.apache.org/r/61052/#comment256803>
Why did you need the `if` check here? It seems like gate is always available?
3rdparty/libprocess/src/process.cpp
Lines 3710 (patched)
<https://reviews.apache.org/r/61052/#comment256804>
Hm.. do you know why we're not using the initializer list for these?
- Benjamin Mahler
On July 22, 2017, 1:08 a.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61052/
> -----------------------------------------------------------
>
> (Updated July 22, 2017, 1:08 a.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Bugs: MESOS-7798
> https://issues.apache.org/jira/browse/MESOS-7798
>
>
> Repository: mesos
>
>
> Description
> -------
>
> In the original implementation we needed a Gate to potentially outlive
> a Process so we stored it in a global map. This patch instead uses
> std::shared_ptr to simplfy the code.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/process.hpp d40179f874754e00b58f271c401650138dc7d01c
> 3rdparty/libprocess/src/gate.hpp a51610e9b20acfe6cd22ea932efd1e6afad84cf2
> 3rdparty/libprocess/src/process.cpp b268cdad776a3ca2a87cbe60eb098bde2a70667c
>
>
> Diff: https://reviews.apache.org/r/61052/diff/1/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>