----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26517/#review57307 -----------------------------------------------------------
src/slave/containerizer/docker.hpp <https://reviews.apache.org/r/26517/#comment97975> Are all directories symlinked? src/slave/containerizer/docker.cpp <https://reviews.apache.org/r/26517/#comment97982> This is unused! src/slave/containerizer/docker.cpp <https://reviews.apache.org/r/26517/#comment97976> Is this container always present? If so, we should be adding a CHECK* and a comment. src/slave/containerizer/docker.cpp <https://reviews.apache.org/r/26517/#comment97977> Is this container always present? If so, we should be adding a CHECK* and a comment as to why this is a valid function precondition. Also, if we're looking up 'directory' why not look up the CommandInfo too? Seems weird to look one up but not the other. Alternatively you could have looked up 'directory' at the call sites to 'fetch'. To be clear, I don't mind looking up the container, but we might as well look up everything from it then to keep people from guessing why we didn't! src/slave/containerizer/docker.cpp <https://reviews.apache.org/r/26517/#comment97978> See my comments above in 'fetch'. In this case though, I'm not convinced that the changes you've made here have any impact since the call sites of 'pull', IIUC, are already extracting the directory from 'container' properly. src/slave/containerizer/docker.cpp <https://reviews.apache.org/r/26517/#comment97979> Why pull this variable out? Unless I'm missing something you should let the compiler do this! ;-) src/tests/docker_containerizer_tests.cpp <https://reviews.apache.org/r/26517/#comment97980> You no longer need to put spaces in between angle brackets! Hooray! src/tests/docker_containerizer_tests.cpp <https://reviews.apache.org/r/26517/#comment97981> You no longer need to put spaces in between angle brackets! Hooray! - Benjamin Hindman On Oct. 17, 2014, 10:57 p.m., Timothy Chen wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/26517/ > ----------------------------------------------------------- > > (Updated Oct. 17, 2014, 10:57 p.m.) > > > Review request for mesos and Benjamin Hindman. > > > Bugs: MESOS-1833 > https://issues.apache.org/jira/browse/MESOS-1833 > > > Repository: mesos-git > > > Description > ------- > > Review: https://reviews.apache.org/r/26517 > > > Diffs > ----- > > src/slave/containerizer/docker.hpp fbbd45d > src/slave/containerizer/docker.cpp 9a29489 > src/tests/docker_containerizer_tests.cpp 67d60a8 > > Diff: https://reviews.apache.org/r/26517/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Timothy Chen > >
