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