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/09/02 15:07:38 UTC
Review Request 62053: Removed garbage collector.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62053/
-----------------------------------------------------------
Review request for mesos, Benjamin Mahler and Jiang Yan Xu.
Bugs: MESOS-7921
https://issues.apache.org/jira/browse/MESOS-7921
Repository: mesos
Description
-------
The garbage collector had at least two bugs:
(1) If someone dispatched `manage()` twice in a row the process we're
waiting for will get overwritten which can wreak havoc depending
on when the calls to `link()` happen.
(2) The garbage collector was deleting after an exited event rather
than actually doing a `wait()`.
The simpler implementation that this patch introduces is to just
delete the process in `ProcessManager::cleanup()`.
Diffs
-----
3rdparty/libprocess/include/Makefile.am c5dc0bb0d2d77987531ead50277940700c62b84f
3rdparty/libprocess/include/process/gc.hpp 603bb8bb084a8d2774ab1077da671f659a22a376
3rdparty/libprocess/include/process/process.hpp 8cca782ae89727bc5570afc4ed96c556f14c8c68
3rdparty/libprocess/src/process.cpp afa53537a5c7d4d8b0f4e3b8e04d7d0f2c4c6631
3rdparty/libprocess/src/tests/process_tests.cpp 8d36600701a795a7fa8d73a844657ff98eee6aa7
Diff: https://reviews.apache.org/r/62053/diff/1/
Testing
-------
make check
Thanks,
Benjamin Hindman
Re: Review Request 62053: Removed garbage collector.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62053/#review184775
-----------------------------------------------------------
Fix it, then Ship it!
3rdparty/libprocess/src/process.cpp
Lines 2818-2820 (original), 2797-2800 (patched)
<https://reviews.apache.org/r/62053/#comment260973>
The unnecessary looping and two `breaks` here makes this a little prone to confusion, how about:
```
// Pick a process (just the first one).
pid = processes.begin()->second.self();
```
to make it more clear we're just grabbing the first one?
3rdparty/libprocess/src/process.cpp
Line 3222 (original), 3184 (patched)
<https://reviews.apache.org/r/62053/#comment260974>
Did you consider defaulting to false given the code below? Seems cleaner?
- Benjamin Mahler
On Sept. 2, 2017, 3:07 p.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62053/
> -----------------------------------------------------------
>
> (Updated Sept. 2, 2017, 3:07 p.m.)
>
>
> Review request for mesos, Benjamin Mahler and Jiang Yan Xu.
>
>
> Bugs: MESOS-7921
> https://issues.apache.org/jira/browse/MESOS-7921
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The garbage collector had at least two bugs:
>
> (1) If someone dispatched `manage()` twice in a row the process we're
> waiting for will get overwritten which can wreak havoc depending
> on when the calls to `link()` happen.
>
> (2) The garbage collector was deleting after an exited event rather
> than actually doing a `wait()`.
>
> The simpler implementation that this patch introduces is to just
> delete the process after doing `ProcessManager::cleanup()`.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/Makefile.am c5dc0bb0d2d77987531ead50277940700c62b84f
> 3rdparty/libprocess/include/process/gc.hpp 603bb8bb084a8d2774ab1077da671f659a22a376
> 3rdparty/libprocess/include/process/process.hpp 8cca782ae89727bc5570afc4ed96c556f14c8c68
> 3rdparty/libprocess/src/process.cpp afa53537a5c7d4d8b0f4e3b8e04d7d0f2c4c6631
> 3rdparty/libprocess/src/tests/process_tests.cpp 8d36600701a795a7fa8d73a844657ff98eee6aa7
>
>
> Diff: https://reviews.apache.org/r/62053/diff/3/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request 62053: Removed garbage collector.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62053/#review184535
-----------------------------------------------------------
Patch looks great!
Reviews applied: [62053]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On Sept. 2, 2017, 3:07 p.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62053/
> -----------------------------------------------------------
>
> (Updated Sept. 2, 2017, 3:07 p.m.)
>
>
> Review request for mesos, Benjamin Mahler and Jiang Yan Xu.
>
>
> Bugs: MESOS-7921
> https://issues.apache.org/jira/browse/MESOS-7921
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The garbage collector had at least two bugs:
>
> (1) If someone dispatched `manage()` twice in a row the process we're
> waiting for will get overwritten which can wreak havoc depending
> on when the calls to `link()` happen.
>
> (2) The garbage collector was deleting after an exited event rather
> than actually doing a `wait()`.
>
> The simpler implementation that this patch introduces is to just
> delete the process after doing `ProcessManager::cleanup()`.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/Makefile.am c5dc0bb0d2d77987531ead50277940700c62b84f
> 3rdparty/libprocess/include/process/gc.hpp 603bb8bb084a8d2774ab1077da671f659a22a376
> 3rdparty/libprocess/include/process/process.hpp 8cca782ae89727bc5570afc4ed96c556f14c8c68
> 3rdparty/libprocess/src/process.cpp afa53537a5c7d4d8b0f4e3b8e04d7d0f2c4c6631
> 3rdparty/libprocess/src/tests/process_tests.cpp 8d36600701a795a7fa8d73a844657ff98eee6aa7
>
>
> Diff: https://reviews.apache.org/r/62053/diff/2/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request 62053: Removed garbage collector.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62053/#review184773
-----------------------------------------------------------
Ship it!
Modulo deletion of the managed process.
- Benjamin Mahler
On Sept. 2, 2017, 3:07 p.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62053/
> -----------------------------------------------------------
>
> (Updated Sept. 2, 2017, 3:07 p.m.)
>
>
> Review request for mesos, Benjamin Mahler and Jiang Yan Xu.
>
>
> Bugs: MESOS-7921
> https://issues.apache.org/jira/browse/MESOS-7921
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The garbage collector had at least two bugs:
>
> (1) If someone dispatched `manage()` twice in a row the process we're
> waiting for will get overwritten which can wreak havoc depending
> on when the calls to `link()` happen.
>
> (2) The garbage collector was deleting after an exited event rather
> than actually doing a `wait()`.
>
> The simpler implementation that this patch introduces is to just
> delete the process after doing `ProcessManager::cleanup()`.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/Makefile.am c5dc0bb0d2d77987531ead50277940700c62b84f
> 3rdparty/libprocess/include/process/gc.hpp 603bb8bb084a8d2774ab1077da671f659a22a376
> 3rdparty/libprocess/include/process/process.hpp 8cca782ae89727bc5570afc4ed96c556f14c8c68
> 3rdparty/libprocess/src/process.cpp afa53537a5c7d4d8b0f4e3b8e04d7d0f2c4c6631
> 3rdparty/libprocess/src/tests/process_tests.cpp 8d36600701a795a7fa8d73a844657ff98eee6aa7
>
>
> Diff: https://reviews.apache.org/r/62053/diff/2/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request 62053: Removed garbage collector.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62053/#review184735
-----------------------------------------------------------
Looks good, just curious why you had to change the semantics of moving the process ownership. Seems worse than before since there's a special case where the caller has to perform the delete if spawn returns `UPID()`?
3rdparty/libprocess/include/process/process.hpp
Lines 574-575 (patched)
<https://reviews.apache.org/r/62053/#comment260934>
Why did you change the semantics here? Previously, this seemed easy to understand since you're either moving ownership or not. Now, when you're moving ownership there is a special case that you always have to deal with?
3rdparty/libprocess/src/process.cpp
Lines 2658-2659 (original), 2651-2652 (patched)
<https://reviews.apache.org/r/62053/#comment260933>
Do we need to give them an async wait? Why is exited not equivalent to that?
3rdparty/libprocess/src/process.cpp
Lines 3404 (patched)
<https://reviews.apache.org/r/62053/#comment260931>
Do you want to say that the invariant right now is that a process always goes through initialization before cleanup? Seems like that's something we might change later (e.g. no point initializing if we already want to terminate it?)
3rdparty/libprocess/src/process.cpp
Line 3484 (original), 3458 (patched)
<https://reviews.apache.org/r/62053/#comment260930>
Can you clarify that it's because a user could rely exited events to know when to delete? Per my comment above, I'm curious why that's wrong.
- Benjamin Mahler
On Sept. 2, 2017, 3:07 p.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62053/
> -----------------------------------------------------------
>
> (Updated Sept. 2, 2017, 3:07 p.m.)
>
>
> Review request for mesos, Benjamin Mahler and Jiang Yan Xu.
>
>
> Bugs: MESOS-7921
> https://issues.apache.org/jira/browse/MESOS-7921
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The garbage collector had at least two bugs:
>
> (1) If someone dispatched `manage()` twice in a row the process we're
> waiting for will get overwritten which can wreak havoc depending
> on when the calls to `link()` happen.
>
> (2) The garbage collector was deleting after an exited event rather
> than actually doing a `wait()`.
>
> The simpler implementation that this patch introduces is to just
> delete the process after doing `ProcessManager::cleanup()`.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/Makefile.am c5dc0bb0d2d77987531ead50277940700c62b84f
> 3rdparty/libprocess/include/process/gc.hpp 603bb8bb084a8d2774ab1077da671f659a22a376
> 3rdparty/libprocess/include/process/process.hpp 8cca782ae89727bc5570afc4ed96c556f14c8c68
> 3rdparty/libprocess/src/process.cpp afa53537a5c7d4d8b0f4e3b8e04d7d0f2c4c6631
> 3rdparty/libprocess/src/tests/process_tests.cpp 8d36600701a795a7fa8d73a844657ff98eee6aa7
>
>
> Diff: https://reviews.apache.org/r/62053/diff/2/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request 62053: Removed garbage collector.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62053/#review184573
-----------------------------------------------------------
FAIL: Mesos tests failed to run
Reviews applied: [62053]
Logs available here: http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/62053/logs
- Mesos Reviewbot Windows
On Sept. 2, 2017, 3:07 p.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62053/
> -----------------------------------------------------------
>
> (Updated Sept. 2, 2017, 3:07 p.m.)
>
>
> Review request for mesos, Benjamin Mahler and Jiang Yan Xu.
>
>
> Bugs: MESOS-7921
> https://issues.apache.org/jira/browse/MESOS-7921
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The garbage collector had at least two bugs:
>
> (1) If someone dispatched `manage()` twice in a row the process we're
> waiting for will get overwritten which can wreak havoc depending
> on when the calls to `link()` happen.
>
> (2) The garbage collector was deleting after an exited event rather
> than actually doing a `wait()`.
>
> The simpler implementation that this patch introduces is to just
> delete the process after doing `ProcessManager::cleanup()`.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/Makefile.am c5dc0bb0d2d77987531ead50277940700c62b84f
> 3rdparty/libprocess/include/process/gc.hpp 603bb8bb084a8d2774ab1077da671f659a22a376
> 3rdparty/libprocess/include/process/process.hpp 8cca782ae89727bc5570afc4ed96c556f14c8c68
> 3rdparty/libprocess/src/process.cpp afa53537a5c7d4d8b0f4e3b8e04d7d0f2c4c6631
> 3rdparty/libprocess/src/tests/process_tests.cpp 8d36600701a795a7fa8d73a844657ff98eee6aa7
>
>
> Diff: https://reviews.apache.org/r/62053/diff/2/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>