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 2021/01/12 19:22:01 UTC
Review Request 73137: Avoid using the libev default loop.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/73137/
-----------------------------------------------------------
Review request for mesos and Ilya Pronin.
Repository: mesos
Description
-------
The default loop in libev supports child process events, which we
don't want and explicitly ignore. We can just use dynamic loops
instead which allows us to properly destroy the loop when libprocess
finalizes.
This also helps minimize the changes needed once we add support for
multiple loops, since those need to be dynamic loops as well.
Diffs
-----
3rdparty/libprocess/src/posix/libev/libev.cpp b38e7a0f882a8c24950bdc6fd74a4d25fc68549e
Diff: https://reviews.apache.org/r/73137/diff/1/
Testing
-------
make check
Thanks,
Benjamin Mahler
Re: Review Request 73137: Avoid using the libev default loop.
Posted by Benjamin Mahler <bm...@apache.org>.
> On Jan. 13, 2021, 7:10 p.m., Ilya Pronin wrote:
> > 3rdparty/libprocess/src/posix/libev/libev.cpp
> > Lines 58 (patched)
> > <https://reviews.apache.org/r/73137/diff/1/?file=2244169#file2244169line58>
> >
> > May be worth providing a default `false` value for `triggered` instead of relying on `Latch` alawys being created using value initialization.
Yes, good point!
> On Jan. 13, 2021, 7:10 p.m., Ilya Pronin wrote:
> > 3rdparty/libprocess/src/posix/libev/libev.cpp
> > Lines 200 (patched)
> > <https://reviews.apache.org/r/73137/diff/1/?file=2244169#file2244169line212>
> >
> > Seems unnecessary.
Yeah, I just wanted to only have two states:
(1) nullptr
(2) set to something non-null and **valid**
and avoid the third state:
(3) set to something non-null but deleted
Since these live beyond the call to stop() finishing
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/73137/#review222464
-----------------------------------------------------------
On Jan. 12, 2021, 7:22 p.m., Benjamin Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/73137/
> -----------------------------------------------------------
>
> (Updated Jan. 12, 2021, 7:22 p.m.)
>
>
> Review request for mesos and Ilya Pronin.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The default loop in libev supports child process events, which we
> don't want and explicitly ignore. We can just use dynamic loops
> instead which allows us to properly destroy the loop when libprocess
> finalizes.
>
> This also helps minimize the changes needed once we add support for
> multiple loops, since those need to be dynamic loops as well.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/posix/libev/libev.cpp b38e7a0f882a8c24950bdc6fd74a4d25fc68549e
>
>
> Diff: https://reviews.apache.org/r/73137/diff/1/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Mahler
>
>
Re: Review Request 73137: Avoid using the libev default loop.
Posted by Ilya Pronin <ip...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/73137/#review222464
-----------------------------------------------------------
Ship it!
3rdparty/libprocess/src/posix/libev/libev.cpp
Lines 58 (patched)
<https://reviews.apache.org/r/73137/#comment311538>
May be worth providing a default `false` value for `triggered` instead of relying on `Latch` alawys being created using value initialization.
3rdparty/libprocess/src/posix/libev/libev.cpp
Lines 63 (patched)
<https://reviews.apache.org/r/73137/#comment311539>
Move this closer to the `loop` declaration?
3rdparty/libprocess/src/posix/libev/libev.cpp
Lines 200 (patched)
<https://reviews.apache.org/r/73137/#comment311540>
Seems unnecessary.
- Ilya Pronin
On Jan. 12, 2021, 11:22 a.m., Benjamin Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/73137/
> -----------------------------------------------------------
>
> (Updated Jan. 12, 2021, 11:22 a.m.)
>
>
> Review request for mesos and Ilya Pronin.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The default loop in libev supports child process events, which we
> don't want and explicitly ignore. We can just use dynamic loops
> instead which allows us to properly destroy the loop when libprocess
> finalizes.
>
> This also helps minimize the changes needed once we add support for
> multiple loops, since those need to be dynamic loops as well.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/posix/libev/libev.cpp b38e7a0f882a8c24950bdc6fd74a4d25fc68549e
>
>
> Diff: https://reviews.apache.org/r/73137/diff/1/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Mahler
>
>
Re: Review Request 73137: Avoid using the libev default loop.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/73137/
-----------------------------------------------------------
(Updated Jan. 20, 2021, 9:28 p.m.)
Review request for mesos and Ilya Pronin.
Changes
-------
Updates based on Ilya's review.
Repository: mesos
Description
-------
The default loop in libev supports child process events, which we
don't want and explicitly ignore. We can just use dynamic loops
instead which allows us to properly destroy the loop when libprocess
finalizes.
This also helps minimize the changes needed once we add support for
multiple loops, since those need to be dynamic loops as well.
Diffs (updated)
-----
3rdparty/libprocess/src/posix/libev/libev.cpp b38e7a0f882a8c24950bdc6fd74a4d25fc68549e
Diff: https://reviews.apache.org/r/73137/diff/2/
Changes: https://reviews.apache.org/r/73137/diff/1-2/
Testing
-------
make check
Thanks,
Benjamin Mahler