----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29889/#review81717 -----------------------------------------------------------
docs/configuration.md <https://reviews.apache.org/r/29889/#comment132142> . And can we add more documentation here to explain why you might want to set this to false and what the consequences are? src/docker/docker.hpp <https://reviews.apache.org/r/29889/#comment132143> While you're updating this line, s/> >/>>/ please. src/docker/docker.cpp <https://reviews.apache.org/r/29889/#comment132145> s/!// src/docker/docker.cpp <https://reviews.apache.org/r/29889/#comment132144> s/if(/if (/ Also, let's pull this out as a constant for now, with a TODO to make it a flag. src/docker/docker.cpp <https://reviews.apache.org/r/29889/#comment132146> s/.// Here and everywhere else. src/docker/executor.cpp <https://reviews.apache.org/r/29889/#comment132150> Why not just make 'run' be the class member field? src/docker/executor.cpp <https://reviews.apache.org/r/29889/#comment132151> Why not CopyFrom? src/docker/executor.cpp <https://reviews.apache.org/r/29889/#comment132154> Please add a comment for why you're doing this, and I prefer this instead: // Comment here why need to make copy. Future<Nothing>(run.get()).discard(); src/slave/containerizer/docker.hpp <https://reviews.apache.org/r/29889/#comment132157> s/id/ID/ Here and everywhere else please. src/slave/containerizer/docker.hpp <https://reviews.apache.org/r/29889/#comment132159> Comment me!! Actually, this looks like dead code. ;-) src/slave/containerizer/docker.cpp <https://reviews.apache.org/r/29889/#comment132161> s/still to/still have to/ src/slave/containerizer/docker.cpp <https://reviews.apache.org/r/29889/#comment132162> Try<Docker*> create = Docker::create(flags.docker); if (create.isError()) { return Error("Failed to create Docker: " + create.error()); } Shared<Docker> docker(create.get()); - Benjamin Hindman On April 22, 2015, 10:50 p.m., Timothy Chen wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/29889/ > ----------------------------------------------------------- > > (Updated April 22, 2015, 10:50 p.m.) > > > Review request for mesos, Benjamin Hindman, Bernd Mathiske, and Till > Toenshoff. > > > 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 93c7c8a > src/docker/docker.hpp 3ebbc1f > src/docker/docker.cpp 3a485a2 > src/docker/executor.cpp PRE-CREATION > src/slave/containerizer/docker.hpp 0d5289c > src/slave/containerizer/docker.cpp f9fb078 > src/slave/flags.hpp d3b1ce1 > src/slave/flags.cpp d0932b0 > src/tests/docker_containerizer_tests.cpp b119a17 > > Diff: https://reviews.apache.org/r/29889/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Timothy Chen > >
