----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38137/#review100449 -----------------------------------------------------------
Ship it! Thanks for being patient. Please commit this! src/slave/containerizer/provisioner/docker/local_puller.hpp (line 40) <https://reviews.apache.org/r/38137/#comment157643> s/docker_discovery_local_dir/docker_local_archives_dir/ I suggested the flag name based on the suggestion about ArchivesPuller, I see that you are keeping the name LocalPuller (because we are not supporting remote archives right now) but changed the flag. Maybe s/LocalPuller/LocalArchivesPuller/ below for now to be consistent? We can revisit other archives locations after https://issues.apache.org/jira/browse/MESOS-3511. src/slave/containerizer/provisioner/docker/local_puller.hpp (line 41) <https://reviews.apache.org/r/38137/#comment157644> s/images saved as tar with the name as the image name with tag/images saved as tars with file names in the form of `<repo>:<tag>.tar`. We better be consistent with repo vs. name in our code and documentation so it's less confusing than docker docs. :) src/slave/containerizer/provisioner/docker/local_puller.cpp (line 279) <https://reviews.apache.org/r/38137/#comment157657> s/directory directory/directory/ src/slave/containerizer/provisioner/docker/paths.hpp (line 47) <https://reviews.apache.org/r/38137/#comment157658> Two blank lines here and elsewhere in this file beause they are outside a class. src/slave/containerizer/provisioner/docker/paths.hpp (line 57) <https://reviews.apache.org/r/38137/#comment157660> s/staging/archivePath/ or s/staging/archiveDir/ This was suggested in my last review. I think the easiest way to explain this bunch of methods is that: given an `archivePath` which is the base dir of the untarred archive, return me the path to the element inside the archive. src/slave/containerizer/provisioner/docker/puller.hpp (line 42) <https://reviews.apache.org/r/38137/#comment157661> s/the directory the puller put its changeset to/the directory where the puller puts its changeset/ src/slave/containerizer/provisioner/docker/puller.hpp (line 57) <https://reviews.apache.org/r/38137/#comment157662> s/returns/return/ to be consistent with the use of first person verb form in this paragraph. src/slave/containerizer/provisioner/docker/puller.hpp (line 74) <https://reviews.apache.org/r/38137/#comment157663> Kill line. - Jiang Yan Xu On Sept. 24, 2015, 1:02 p.m., Timothy Chen wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/38137/ > ----------------------------------------------------------- > > (Updated Sept. 24, 2015, 1:02 p.m.) > > > Review request for mesos, Jie Yu, Jojy Varghese, Till Toenshoff, and Jiang > Yan Xu. > > > Repository: mesos > > > Description > ------- > > Joining all the commits around provisioner and local store into one review so > it's easier to review, as patches > are changing code on top of each other. > > All the commits are going to committed together. > > > Diffs > ----- > > src/Makefile.am 776483b > src/slave/containerizer/provisioner/docker/local_puller.hpp PRE-CREATION > src/slave/containerizer/provisioner/docker/local_puller.cpp PRE-CREATION > src/slave/containerizer/provisioner/docker/message.hpp PRE-CREATION > src/slave/containerizer/provisioner/docker/message.proto PRE-CREATION > src/slave/containerizer/provisioner/docker/metadata_manager.hpp > PRE-CREATION > src/slave/containerizer/provisioner/docker/metadata_manager.cpp > PRE-CREATION > src/slave/containerizer/provisioner/docker/paths.hpp PRE-CREATION > src/slave/containerizer/provisioner/docker/paths.cpp PRE-CREATION > src/slave/containerizer/provisioner/docker/puller.hpp PRE-CREATION > src/slave/containerizer/provisioner/docker/puller.cpp PRE-CREATION > src/slave/containerizer/provisioner/docker/store.hpp PRE-CREATION > src/slave/containerizer/provisioner/docker/store.cpp PRE-CREATION > src/slave/containerizer/provisioner/store.cpp 35d1199 > src/slave/flags.hpp 3f6601a > src/slave/flags.cpp 8792162 > src/tests/containerizer/provisioner_docker_tests.cpp 1b0c304 > > Diff: https://reviews.apache.org/r/38137/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Timothy Chen > >
