Re: Review Request 26580: Memory cleanup: libprocess join worker threads

[Joris Van Remoortere]

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

(Updated Oct. 14, 2014, 9:44 p.m.)


Review request for mesos, Benjamin Hindman and Niklas Nielsen.


Changes
---

Address Dominic's issues.


Repository: mesos-git


Description
---

Terminate the gc;
Finalize the ProcessManager (picking up -idealy 0- left-over processes);
Join on the worker threads that were dispatched.


Diffs (updated)
-

  3rdparty/libprocess/src/process.cpp 85fb995 

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


Testing
---

make check
support/mesos-style.py


Thanks,

Joris Van Remoortere

Re: Review Request 26580: Memory cleanup: libprocess join worker threads

[Dominic Hamon]

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



3rdparty/libprocess/src/process.cpp


you don't need the else here, given the 'if' has a FATAL.



3rdparty/libprocess/src/process.cpp


if we have a process with a long-running (infinite) loop will this block 
shutdown? should we have a timeout here?



3rdparty/libprocess/src/process.cpp


is this really fatal? ie - should we try to continue joining the other 
threads but log as an error that we failed to join some?


- Dominic Hamon


On Oct. 13, 2014, 11:26 a.m., Joris Van Remoortere wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26580/
> ---
> 
> (Updated Oct. 13, 2014, 11:26 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Niklas Nielsen.
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> Terminate the gc;
> Finalize the ProcessManager (picking up -idealy 0- left-over processes);
> Join on the worker threads that were dispatched.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 85fb995 
> 
> Diff: https://reviews.apache.org/r/26580/diff/
> 
> 
> Testing
> ---
> 
> make check
> support/mesos-style.py
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>

Re: Review Request 26580: Memory cleanup: libprocess join worker threads

[Joris Van Remoortere]

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

(Updated Oct. 13, 2014, 6:26 p.m.)


Review request for mesos, Benjamin Hindman and Niklas Nielsen.


Changes
---

use a static inside a function body to rely on standardized c++ initialization 
order.


Repository: mesos-git


Description
---

Terminate the gc;
Finalize the ProcessManager (picking up -idealy 0- left-over processes);
Join on the worker threads that were dispatched.


Diffs (updated)
-

  3rdparty/libprocess/src/process.cpp 85fb995 

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


Testing
---

make check
support/mesos-style.py


Thanks,

Joris Van Remoortere

Re: Review Request 26580: Memory cleanup: libprocess join worker threads

[Dominic Hamon]

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



3rdparty/libprocess/src/process.cpp


so it turns out i gave you bad advice. i checked, and unique_ptr is also 
non-POD and thus has static destruction order issues.

"If you need a static or global variable of a class type, consider 
initializing a pointer (which will never be freed), from either your main() 
function or from pthread_once(). Note that this must be a raw pointer, not a 
"smart" pointer, since the smart pointer's destructor will have the 
order-of-destructor issue that we are trying to avoid." (the google c++ style 
guide).

i think this will need to be a raw pointer after all, or an instance 
variable.


- Dominic Hamon


On Oct. 13, 2014, 11:10 a.m., Joris Van Remoortere wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26580/
> ---
> 
> (Updated Oct. 13, 2014, 11:10 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Niklas Nielsen.
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> Terminate the gc;
> Finalize the ProcessManager (picking up -idealy 0- left-over processes);
> Join on the worker threads that were dispatched.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 85fb995 
> 
> Diff: https://reviews.apache.org/r/26580/diff/
> 
> 
> Testing
> ---
> 
> make check
> support/mesos-style.py
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>

Re: Review Request 26580: Memory cleanup: libprocess join worker threads

[Joris Van Remoortere]

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

(Updated Oct. 13, 2014, 6:10 p.m.)


Review request for mesos, Benjamin Hindman and Niklas Nielsen.


Changes
---

using std::unique_ptr for list of worker pthreads.


Repository: mesos-git


Description
---

Terminate the gc;
Finalize the ProcessManager (picking up -idealy 0- left-over processes);
Join on the worker threads that were dispatched.


Diffs (updated)
-

  3rdparty/libprocess/src/process.cpp 85fb995 

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


Testing
---

make check
support/mesos-style.py


Thanks,

Joris Van Remoortere

Re: Review Request 26580: Memory cleanup: libprocess join worker threads

[Dominic Hamon]

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



3rdparty/libprocess/src/process.cpp


this is non-POD and so can't be statically constructed: 
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Static_and_Global_Variables.

you could store a list* or std::unique_ptr> or 
return a pointer from a function:

static list* workerThreads()
{
  static list workerThreads;
  return *workerThreads;
}


- Dominic Hamon


On Oct. 10, 2014, 4:38 p.m., Joris Van Remoortere wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26580/
> ---
> 
> (Updated Oct. 10, 2014, 4:38 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Niklas Nielsen.
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> Terminate the gc;
> Finalize the ProcessManager (picking up -idealy 0- left-over processes);
> Join on the worker threads that were dispatched.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 85fb995 
> 
> Diff: https://reviews.apache.org/r/26580/diff/
> 
> 
> Testing
> ---
> 
> make check
> support/mesos-style.py
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>

Re: Review Request 26580: Memory cleanup: libprocess join worker threads

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [26578, 26580]

All tests passed.

- Mesos ReviewBot


On Oct. 10, 2014, 11:38 p.m., Joris Van Remoortere wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26580/
> ---
> 
> (Updated Oct. 10, 2014, 11:38 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Niklas Nielsen.
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> Terminate the gc;
> Finalize the ProcessManager (picking up -idealy 0- left-over processes);
> Join on the worker threads that were dispatched.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 85fb995 
> 
> Diff: https://reviews.apache.org/r/26580/diff/
> 
> 
> Testing
> ---
> 
> make check
> support/mesos-style.py
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>

Re: Review Request 26580: Memory cleanup: libprocess join worker threads

[Joris Van Remoortere]

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

(Updated Oct. 10, 2014, 11:38 p.m.)


Review request for mesos, Benjamin Hindman and Niklas Nielsen.


Changes
---

Introduce finalize() on ProcessManager so that we can terminate outstanding 
processes without destroying our runqueue.


Repository: mesos-git


Description (updated)
---

Terminate the gc;
Finalize the ProcessManager (picking up -idealy 0- left-over processes);
Join on the worker threads that were dispatched.


Diffs (updated)
-

  3rdparty/libprocess/src/process.cpp 85fb995 

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


Testing
---

make check
support/mesos-style.py


Thanks,

Joris Van Remoortere