> On May 24, 2015, 1:55 a.m., Benjamin Hindman wrote:
> > src/docker/docker.hpp, line 64
> > <https://reviews.apache.org/r/29889/diff/13/?file=970842#file970842line64>
> >
> >     Can you add a small comment explaining when 'started' would be 'false'? 
> > I'm guessing you want to use 'started' here to help figure out when a 
> > container has "started" when you do an attached 'docker run', but I don't 
> > know what 'started' implies, and how it is different than checking when 
> > 'pid.isSome()'?

Good point, I cannot use pid because at the time when I inspect again the 
container might already finished and there is no pid available.
So the easiest way to know if the container has actually started is by 
examining the startedAt


> On May 24, 2015, 1:55 a.m., Benjamin Hindman wrote:
> > src/slave/containerizer/docker.hpp, lines 462-464
> > <https://reviews.apache.org/r/29889/diff/13/?file=970848#file970848line462>
> >
> >     Any reason not to make these Option as well? It seems a little wierd to 
> > potentially read these and get garbage (or missing) data in the case that 
> > they were actually passed into the constructor as 'None'.

Since we always set it to something in the constructor, I thought there is no 
point to set them to Option. If you look in the constructor code you'll see 
that.


> On May 24, 2015, 1:55 a.m., Benjamin Hindman wrote:
> > src/slave/containerizer/docker.cpp, lines 845-848
> > <https://reviews.apache.org/r/29889/diff/13/?file=970849#file970849line845>
> >
> >     Why do you need to associate the promise within the 'onAny' callback? 
> > You should just be able to do:
> >     
> >     
> >     Future<Docker::Container> inspect =
> >         docker->inspect(containerName, slave::DOCKER_INSPECT_DELAY);
> >     
> >     promise->associate(inspect);
> >     
> >     
> >     The other thing that is a bit mysterious to me here is why you actually 
> > want/need an extra Promise? Seems like you can just do:
> >     
> >     run.onFailed([=](const string& failure) {
> >       inspect.discard();
> >     });
> >     
> >     return inspect;
> >     
> >     
> >     Maybe it's because you don't want a 'discarded' future to propagate out 
> > from 'launchExecutorContainer'? In which case, I'd comment as much, i.e., 
> > that you explicitly want a failed future rather than let a discarded future 
> > propagate and clearly explain why (hopefully there isn't other code relying 
> > on it someplace because that's definitely a global invariant that we should 
> > avoid!).

I think you're right the simplified version is all I need, in the beginning I 
made a promise because I was thinking I might need to call inspect again later 
as I was thinking to keep inpsect simple and made the retries outside. But now 
with retries happening in inspect, this is a lot easier.


> On May 24, 2015, 1:55 a.m., Benjamin Hindman wrote:
> > src/slave/containerizer/docker.cpp, line 912
> > <https://reviews.apache.org/r/29889/diff/13/?file=970849#file970849line912>
> >
> >     So this version is correctly working? Just wanted to call this out 
> > explicitly because the other one is not and that makes me wonder if maybe 
> > this code path isn't being tested?

No this actually works if I just pass the flags straight without slicing, but 
in the other path where I originally store the flags in a 
Option<flags::FlagsBase> and then pass that into subprocess actually gives me 
bad access from time to time. I didn't want to hard code the type in dockerRun 
to be docker::Flags because I thought docker::run should take a generic 
flagbase in case it wants to pass any arbitrary flags in.


- Timothy


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


On May 23, 2015, 6:14 a.m., Timothy Chen wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29889/
> -----------------------------------------------------------
> 
> (Updated May 23, 2015, 6:14 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Bernd Mathiske, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-2115
>     https://issues.apache.org/jira/browse/MESOS-2115
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This is a one mega patch and contains many reviews that's already on rb.
> 
> This review is not meant to be merged, only provided for easier review.
> 
> 
> Diffs
> -----
> 
>   Dockerfile 35abf25 
>   docs/configuration.md 54c4e31 
>   docs/docker-containerizer.md a5438b7 
>   src/Makefile.am 34755cf 
>   src/docker/docker.hpp 3ebbc1f 
>   src/docker/docker.cpp 3a485a2 
>   src/docker/executor.hpp PRE-CREATION 
>   src/docker/executor.cpp PRE-CREATION 
>   src/slave/constants.hpp fd1c1ab 
>   src/slave/constants.cpp 2a99b11 
>   src/slave/containerizer/docker.hpp ed4ee19 
>   src/slave/containerizer/docker.cpp 408a443 
>   src/slave/flags.hpp 5c57478 
>   src/slave/flags.cpp b5e2518 
>   src/tests/docker_containerizer_tests.cpp 154bf98 
>   src/tests/docker_tests.cpp 5520c58 
> 
> Diff: https://reviews.apache.org/r/29889/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Reply via email to