----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29334/#review68559 -----------------------------------------------------------
src/slave/containerizer/docker.hpp <https://reviews.apache.org/r/29334/#comment112866> At this compleity level, the comments here have begun to look like an anti-pattern that might create unwanted precedent for others to mimic. IMHO this was already the case before this patch, but the current additions exacerbate it. The naming launchInContainer helps, but it is still in line with the expected style? It seems to me that the expected style breaks down here. Ideas? src/slave/containerizer/docker.hpp <https://reviews.apache.org/r/29334/#comment112871> IMHO readability is subverted by prolonging the underscore scheme when there is no strict series of continuations. This code would be so much more easy to read if there were descriptive method names. Absent these, please add comments that summarize the purpose and general approach of these methods in places such as this one. src/slave/containerizer/docker.cpp <https://reviews.apache.org/r/29334/#comment112870> Here one can see that we are dealing with a more general graph than a series. How can one know why ___launch (3 underscores) is not needed in this branch? And how can one know what ___launch, ____launch, _____launch do in the first place? One must read them all, memorize them, and then read this stretch of code again. Ideas? src/slave/containerizer/docker.cpp <https://reviews.apache.org/r/29334/#comment112868> When using names such as "___launch" without any trace of what the continuation specified actually does, then we need a comment somewhere to explain purpose and general approach. This can be minimal, but putting nothing at all forces inefficient depth-first code reading. src/slave/containerizer/docker.cpp <https://reviews.apache.org/r/29334/#comment112872> I would group this differently: ContainerInfo::DockerInfo dockerInfo; dockerInfo.set_image(flags.docker_mesos_image.get()); ContainerInfo containerInfo; containerInfo.mutable_docker()->CopyFrom(dockerInfo); src/slave/containerizer/docker.cpp <https://reviews.apache.org/r/29334/#comment112873> "mesos-docker-executor" comes out of nowhere. Declaring it as a constant (possibly, but not necessarily in this file) would provide a natural way to link to background information (e.g. what exactly this program does and where to find it). This info could then be found trivially by looking up said declaration, where one would place some explanations. src/slave/containerizer/docker.cpp <https://reviews.apache.org/r/29334/#comment112874> It would be clearer not reusing the volume variable here. src/slave/containerizer/docker.cpp <https://reviews.apache.org/r/29334/#comment112875> Not checking the outcome of this? src/slave/containerizer/docker.cpp <https://reviews.apache.org/r/29334/#comment112876> not -> not have on -> in has -> had been executor -> executors - Bernd Mathiske On Jan. 16, 2015, 5:36 p.m., Timothy Chen wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/29334/ > ----------------------------------------------------------- > > (Updated Jan. 16, 2015, 5:36 p.m.) > > > Review request for mesos, Benjamin Hindman and Bernd Mathiske. > > > Repository: mesos-git > > > Description > ------- > > Add option to launch docker containers with helper containers. > > > Diffs > ----- > > src/slave/containerizer/docker.hpp b7bf54a > src/slave/containerizer/docker.cpp 5f4b4ce > > Diff: https://reviews.apache.org/r/29334/diff/ > > > Testing > ------- > > make, tests are fixed in next commit > > > Thanks, > > Timothy Chen > >
