Re: Review Request 62053: Removed garbage collector.

[Benjamin Mahler]

---
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)


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)


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.

[Benjamin Mahler]

---
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.

[Benjamin Mahler]

---
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)


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)


Do we need to give them an async wait? Why is exited not equivalent to that?



3rdparty/libprocess/src/process.cpp
Lines 3404 (patched)


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)


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.

[Mesos Reviewbot Windows]

---
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
> 
>

Re: Review Request 62053: Removed garbage collector.

[Mesos Reviewbot]

---
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
> 
>

Review Request 62053: Removed garbage collector.

[Benjamin Hindman]

---
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