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/01/08 07:49:15 UTC

Review Request 55321: Introduced process::after.

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

Review request for mesos and Jie Yu.


Repository: mesos


Description
-------

Introduced process::after.


Diffs
-----

  3rdparty/libprocess/Makefile.am 5e7fdd06ccbed50f248c81e9df1651a3702e7add 
  3rdparty/libprocess/include/Makefile.am 1d17edd933562849b35740f3935685c8eb154440 
  3rdparty/libprocess/include/process/after.hpp PRE-CREATION 
  3rdparty/libprocess/src/tests/after_tests.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/55321/diff/


Testing
-------

make check


Thanks,

Benjamin Hindman

Re: Review Request 55321: Introduced process::after.

Posted by Benjamin Hindman <be...@berkeley.edu>.


> On Feb. 2, 2017, 11:19 a.m., Alexander Rukletsov wrote:
> > 3rdparty/libprocess/include/process/after.hpp, line 51
> > <https://reviews.apache.org/r/55321/diff/1/?file=1599672#file1599672line51>
> >
> >     We keep two copies of a shared pointer to `promise` in an `onDiscard` callback (one directly and one via `timer`). It seems that all code paths in `after` do clear `onDiscardCallbacks` and hence destruct the promise. But I'd have tests proving that rather than relying on my mental compiling abilities : ).

Suggestion on how to create such a test? I've added a comment for now (per Benjamin's request).


- Benjamin


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


On Jan. 8, 2017, 7:49 a.m., Benjamin Hindman wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55321/
> -----------------------------------------------------------
> 
> (Updated Jan. 8, 2017, 7:49 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Introduced process::after.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/Makefile.am 5e7fdd06ccbed50f248c81e9df1651a3702e7add 
>   3rdparty/libprocess/include/Makefile.am 1d17edd933562849b35740f3935685c8eb154440 
>   3rdparty/libprocess/include/process/after.hpp PRE-CREATION 
>   3rdparty/libprocess/src/tests/after_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/55321/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

Re: Review Request 55321: Introduced process::after.

Posted by Alexander Rukletsov <ru...@gmail.com>.

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




3rdparty/libprocess/include/Makefile.am (lines 15 - 16)
<https://reviews.apache.org/r/55321/#comment235472>

    Swap these.



3rdparty/libprocess/include/process/after.hpp (line 51)
<https://reviews.apache.org/r/55321/#comment235474>

    We keep two copies of a shared pointer to `promise` in an `onDiscard` callback (one directly and one via `timer`). It seems that all code paths in `after` do clear `onDiscardCallbacks` and hence destruct the promise. But I'd have tests proving that rather than relying on my mental compiling abilities : ).


- Alexander Rukletsov


On Jan. 8, 2017, 7:49 a.m., Benjamin Hindman wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55321/
> -----------------------------------------------------------
> 
> (Updated Jan. 8, 2017, 7:49 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Introduced process::after.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/Makefile.am 5e7fdd06ccbed50f248c81e9df1651a3702e7add 
>   3rdparty/libprocess/include/Makefile.am 1d17edd933562849b35740f3935685c8eb154440 
>   3rdparty/libprocess/include/process/after.hpp PRE-CREATION 
>   3rdparty/libprocess/src/tests/after_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/55321/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

Re: Review Request 55321: Introduced process::after.

Posted by Benjamin Hindman <be...@berkeley.edu>.


> On Feb. 2, 2017, 11:26 a.m., Gast�n Kleiman wrote:
> > 3rdparty/libprocess/Makefile.am, line 237
> > <https://reviews.apache.org/r/55321/diff/1/?file=1599670#file1599670line237>
> >
> >     Shouldn't we update the cmake lists as well?

Thanks!


- Benjamin


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


On Jan. 8, 2017, 7:49 a.m., Benjamin Hindman wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55321/
> -----------------------------------------------------------
> 
> (Updated Jan. 8, 2017, 7:49 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Introduced process::after.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/Makefile.am 5e7fdd06ccbed50f248c81e9df1651a3702e7add 
>   3rdparty/libprocess/include/Makefile.am 1d17edd933562849b35740f3935685c8eb154440 
>   3rdparty/libprocess/include/process/after.hpp PRE-CREATION 
>   3rdparty/libprocess/src/tests/after_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/55321/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

Re: Review Request 55321: Introduced process::after.

Posted by Gastón Kleiman <ga...@mesosphere.com>.

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




3rdparty/libprocess/Makefile.am (line 237)
<https://reviews.apache.org/r/55321/#comment235475>

    Shouldn't we update the cmake lists as well?


- Gast�n Kleiman


On Jan. 8, 2017, 7:49 a.m., Benjamin Hindman wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55321/
> -----------------------------------------------------------
> 
> (Updated Jan. 8, 2017, 7:49 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Introduced process::after.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/Makefile.am 5e7fdd06ccbed50f248c81e9df1651a3702e7add 
>   3rdparty/libprocess/include/Makefile.am 1d17edd933562849b35740f3935685c8eb154440 
>   3rdparty/libprocess/include/process/after.hpp PRE-CREATION 
>   3rdparty/libprocess/src/tests/after_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/55321/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

Re: Review Request 55321: Introduced process::after.

Posted by Gastón Kleiman <ga...@mesosphere.com>.

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




3rdparty/libprocess/include/process/after.hpp (line 25)
<https://reviews.apache.org/r/55321/#comment235320>

    s/let's/lets/


- Gast�n Kleiman


On Jan. 8, 2017, 7:49 a.m., Benjamin Hindman wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55321/
> -----------------------------------------------------------
> 
> (Updated Jan. 8, 2017, 7:49 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Introduced process::after.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/Makefile.am 5e7fdd06ccbed50f248c81e9df1651a3702e7add 
>   3rdparty/libprocess/include/Makefile.am 1d17edd933562849b35740f3935685c8eb154440 
>   3rdparty/libprocess/include/process/after.hpp PRE-CREATION 
>   3rdparty/libprocess/src/tests/after_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/55321/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

Re: Review Request 55321: Introduced process::after.

Posted by Benjamin Hindman <be...@berkeley.edu>.


> On Feb. 1, 2017, 4:38 p.m., Benjamin Bannier wrote:
> > 3rdparty/libprocess/include/process/after.hpp, lines 50-55
> > <https://reviews.apache.org/r/55321/diff/1/?file=1599672#file1599672line50>
> >
> >     I was trying to wrap my head around whether this created a bad circular reference `promise -> future -> discardCallback -> promise` and ultimately a leak of `Future::data` or not. Are we sure this does not leak when a user e.g., calls it like this?,
> >     
> >         after(Seconds(10)); // return value ignored.
> >     
> >     I believe adding a clarifyinh comment, or even better a test would be great.

I've added a comment Benjamin. Not sure how I can test this, did you have something in mind?


- Benjamin


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


On Jan. 8, 2017, 7:49 a.m., Benjamin Hindman wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55321/
> -----------------------------------------------------------
> 
> (Updated Jan. 8, 2017, 7:49 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Introduced process::after.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/Makefile.am 5e7fdd06ccbed50f248c81e9df1651a3702e7add 
>   3rdparty/libprocess/include/Makefile.am 1d17edd933562849b35740f3935685c8eb154440 
>   3rdparty/libprocess/include/process/after.hpp PRE-CREATION 
>   3rdparty/libprocess/src/tests/after_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/55321/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

Re: Review Request 55321: Introduced process::after.

Posted by Benjamin Bannier <be...@mesosphere.io>.

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




3rdparty/libprocess/include/process/after.hpp (lines 50 - 55)
<https://reviews.apache.org/r/55321/#comment235317>

    I was trying to wrap my head around whether this created a bad circular reference `promise -> future -> discardCallback -> promise` and ultimately a leak of `Future::data` or not. Are we sure this does not leak when a user e.g., calls it like this?,
    
        after(Seconds(10)); // return value ignored.
    
    I believe adding a clarifyinh comment, or even better a test would be great.


- Benjamin Bannier


On Jan. 8, 2017, 8:49 a.m., Benjamin Hindman wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55321/
> -----------------------------------------------------------
> 
> (Updated Jan. 8, 2017, 8:49 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Introduced process::after.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/Makefile.am 5e7fdd06ccbed50f248c81e9df1651a3702e7add 
>   3rdparty/libprocess/include/Makefile.am 1d17edd933562849b35740f3935685c8eb154440 
>   3rdparty/libprocess/include/process/after.hpp PRE-CREATION 
>   3rdparty/libprocess/src/tests/after_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/55321/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

Re: Review Request 55321: Introduced process::after.

Posted by Benjamin Mahler <bm...@apache.org>.

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


Ship it!




Nice to have! I wonder if we still want delay long term:

```
delay(self(), &Self::continuation);

// vs

after(Seconds(5))
  .then(defer(self(), &Self::continuation))
```


3rdparty/libprocess/include/process/after.hpp (lines 16 - 20)
<https://reviews.apache.org/r/55321/#comment235279>

    Looks like you need:
    
    <memory>
    
    <stout/nothing.hpp>



3rdparty/libprocess/src/tests/after_tests.cpp (line 34)
<https://reviews.apache.org/r/55321/#comment235280>

    How about `Milliseconds(1)` like you did below since there doesn't seem to be any need for a specific duration and we don't want to slow down the test. 1 nanosecond also seems to be sufficient for this test?


- Benjamin Mahler


On Jan. 8, 2017, 7:49 a.m., Benjamin Hindman wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55321/
> -----------------------------------------------------------
> 
> (Updated Jan. 8, 2017, 7:49 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Introduced process::after.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/Makefile.am 5e7fdd06ccbed50f248c81e9df1651a3702e7add 
>   3rdparty/libprocess/include/Makefile.am 1d17edd933562849b35740f3935685c8eb154440 
>   3rdparty/libprocess/include/process/after.hpp PRE-CREATION 
>   3rdparty/libprocess/src/tests/after_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/55321/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>