mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Mahler <bmah...@apache.org>
Subject Re: Review Request 62053: Removed garbage collector.
Date Wed, 06 Sep 2017 21:22:51 GMT

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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message