mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Hindman <b...@berkeley.edu>
Subject Re: Review Request 51278: Refactored LinuxLauncher to be nested container aware.
Date Tue, 27 Sep 2016 05:44:32 GMT


> On Sept. 25, 2016, 10:32 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/linux_launcher.cpp, line 614
> > <https://reviews.apache.org/r/51278/diff/3/?file=1509809#file1509809line614>
> >
> >     using executor_pid here is weird. I think we should rethink how we should `ContainerStatus`
when we have nested containers. Should we rename it to `pid`? Sounds like 'executor_pid' is
not very precise.
> >     
> >     Another to consider is: in the future, user might want to get the pid of it's
actual process, not the 'init' pid we forked. What should we call that?

I'm going to drop this for now, let's iterate on this together in a subsequent review.


> On Sept. 25, 2016, 10:32 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/linux_launcher.cpp, lines 565-571
> > <https://reviews.apache.org/r/51278/diff/3/?file=1509809#file1509809line565>
> >
> >     I won't worry about this case because the containerizer will make sure cleanup/destroy
is only called once for launcher/isolator/provisioner.
> >     
> >     But I like the fact we remove the container from `containers` map so that other
calls to launcher will be properly handled (e.g., status).

I improved the comment but still mentioned this because we're missing proper separation of
concerns by assuming that we know how the caller is calling this function. I didn't write
that code, someone else did, I'm just exposing this functionality through an interface!


- Benjamin


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


On Sept. 25, 2016, 4:08 p.m., Benjamin Hindman wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51278/
> -----------------------------------------------------------
> 
> (Updated Sept. 25, 2016, 4:08 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, and Kevin Klues.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Refactored LinuxLauncher to be nested container aware.
> 
> 
> Diffs
> -----
> 
>   src/slave/containerizer/mesos/launch.hpp 859990cb85e9e8c06400397256cfc512f0811800 
>   src/slave/containerizer/mesos/linux_launcher.hpp 2cd1e4d4b6d9e656c2447a9d3ab8e1f49ba5fffa

>   src/slave/containerizer/mesos/linux_launcher.cpp f27757032cc28be0f0067bed045536431dde4d6c

> 
> Diff: https://reviews.apache.org/r/51278/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>


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