----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38137/#review99772 -----------------------------------------------------------
Overall, looks good. The main issue is that 'Store' should not have dependencies on 'Local' puller (see my detailed comments below). src/Makefile.am (line 256) <https://reviews.apache.org/r/38137/#comment156751> Kill one blank line here. src/Makefile.am (line 491) <https://reviews.apache.org/r/38137/#comment156752> Similar to master/resgistry.proto, we should put this proto file under `slave/containerizer/provisioner/docker.proto` or `slave/containerizer/provisioner/docker/message.proto` ? src/slave/containerizer/provisioner.cpp (lines 49 - 51) <https://reviews.apache.org/r/38137/#comment156758> You need to do a rebase. This file has been moved to src/containerizer/provisioner/provisioner.cpp. Or you just forgot to remove this file from the patch? src/slave/containerizer/provisioner/docker/local_puller.cpp (line 61) <https://reviews.apache.org/r/38137/#comment156873> I don't see the implementation of this method? src/slave/containerizer/provisioner/docker/local_puller.cpp (line 122) <https://reviews.apache.org/r/38137/#comment156871> Insert a blank line above. src/slave/containerizer/provisioner/docker/local_puller.cpp (lines 146 - 151) <https://reviews.apache.org/r/38137/#comment156874> Do you want to kill the 'tar' process when slave exits? You may want to take a look at how we setup death signal for 'perf' (src/linuc/perf.cpp). It's ok you don't want to address that in this patch. Please add a TODO somewhere. src/slave/containerizer/provisioner/docker/local_puller.cpp (line 158) <https://reviews.apache.org/r/38137/#comment156876> Can you just capture 'tarPath' here to be more explicit? src/slave/containerizer/provisioner/docker/local_puller.cpp (line 293) <https://reviews.apache.org/r/38137/#comment156880> Insert a blank line above. src/slave/containerizer/provisioner/docker/local_puller.cpp (line 321) <https://reviews.apache.org/r/38137/#comment156879> Insert a blank line above. src/slave/containerizer/provisioner/docker/local_puller.cpp (line 326) <https://reviews.apache.org/r/38137/#comment156878> Why do you need to capture '='? src/slave/containerizer/provisioner/docker/local_puller.cpp (line 338) <https://reviews.apache.org/r/38137/#comment156872> Kill a blank line here. src/slave/containerizer/provisioner/docker/message.proto (line 21) <https://reviews.apache.org/r/38137/#comment156836> Remove .docker? src/slave/containerizer/provisioner/docker/message.proto (line 40) <https://reviews.apache.org/r/38137/#comment156838> 2 lines apart. src/slave/containerizer/provisioner/docker/metadata_manager.hpp (lines 86 - 89) <https://reviews.apache.org/r/38137/#comment156864> Please move 'recover()' above 'put'. src/slave/containerizer/provisioner/docker/metadata_manager.cpp (line 51) <https://reviews.apache.org/r/38137/#comment156866> Kill a blank line here. src/slave/containerizer/provisioner/docker/metadata_manager.cpp (lines 57 - 58) <https://reviews.apache.org/r/38137/#comment156865> As we discussed before, please kill this method. Instead, expose the constructor. src/slave/containerizer/provisioner/docker/metadata_manager.cpp (line 89) <https://reviews.apache.org/r/38137/#comment156841> Insert a blank line above. src/slave/containerizer/provisioner/docker/metadata_manager.cpp (line 92) <https://reviews.apache.org/r/38137/#comment156843> Insert a blank line above. src/slave/containerizer/provisioner/docker/metadata_manager.cpp (lines 120 - 121) <https://reviews.apache.org/r/38137/#comment156845> Mind adjust the arguments like the following. It's less jagged:) ``` return dispatch( process.get(), &MetadataManagerProcess::put, name, layerIds); ``` src/slave/containerizer/provisioner/docker/metadata_manager.cpp (line 203) <https://reviews.apache.org/r/38137/#comment156867> Why do you need this? src/slave/containerizer/provisioner/docker/metadata_manager.cpp (line 206) <https://reviews.apache.org/r/38137/#comment156869> Please quote the path (what if path contains spaces?) Also, remove the period in the end (please make sure to fix all occurances in this patch). src/slave/containerizer/provisioner/docker/metadata_manager.cpp (line 207) <https://reviews.apache.org/r/38137/#comment156868> Insert a blank line above. src/slave/containerizer/provisioner/docker/paths.hpp (line 40) <https://reviews.apache.org/r/38137/#comment156870> What's this? src/slave/containerizer/provisioner/docker/store.hpp (lines 22 - 37) <https://reviews.apache.org/r/38137/#comment156770> Please cleanup the header includes here. Remove those that are not needed. src/slave/containerizer/provisioner/docker/store.hpp (lines 59 - 61) <https://reviews.apache.org/r/38137/#comment156769> Swap the order of these two methods. We typically put 'recover' before other methods. src/slave/containerizer/provisioner/docker/store.cpp (line 70) <https://reviews.apache.org/r/38137/#comment156776> No need for process:: prefix. You did use 'using namespace process' in the beginning of this file. Please do a sweep to remove all such occurances. src/slave/containerizer/provisioner/docker/store.cpp (lines 74 - 76) <https://reviews.apache.org/r/38137/#comment156857> Do you still need this? src/slave/containerizer/provisioner/docker/store.cpp (lines 82 - 85) <https://reviews.apache.org/r/38137/#comment156856> Do you still need this? Please do a sweep to fix all such issues. src/slave/containerizer/provisioner/docker/store.cpp (line 170) <https://reviews.apache.org/r/38137/#comment156839> Can you return a Failure instead here? src/slave/containerizer/provisioner/docker/store.cpp (line 172) <https://reviews.apache.org/r/38137/#comment156846> s/dockerName/imageName/? src/slave/containerizer/provisioner/docker/store.cpp (lines 198 - 199) <https://reviews.apache.org/r/38137/#comment156847> Who will cleanup the staging directories? If you haven't planned to do it, mind adding a TODO so that we don't forget. src/slave/containerizer/provisioner/docker/store.cpp (lines 210 - 215) <https://reviews.apache.org/r/38137/#comment156855> Coud you please move this method right below moveLayers so that the code is more readable (i.e., following this continuation ordering). src/slave/containerizer/provisioner/docker/store.cpp (lines 218 - 221) <https://reviews.apache.org/r/38137/#comment156851> Could you please move this method right above ::get() so that related methods are grouped together. src/slave/containerizer/provisioner/docker/store.cpp (line 228) <https://reviews.apache.org/r/38137/#comment156858> Why do you need the 'Nothing()' here? src/slave/containerizer/provisioner/docker/store.cpp (line 256) <https://reviews.apache.org/r/38137/#comment156863> According to your current design, Store is supposed to be agnostic to which puller is being used, right? Therefore, there should be local puller specific code in Store. Shouldn't the puller interface returns a map: imageId -> path_to_fs_layer and the Store just looks at the paths and move them to getImageLayerRootfsPath? src/slave/containerizer/provisioner/docker/store.cpp (line 266) <https://reviews.apache.org/r/38137/#comment156771> Kill one blank line here. - Jie Yu On Sept. 21, 2015, 5:50 p.m., Timothy Chen wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/38137/ > ----------------------------------------------------------- > > (Updated Sept. 21, 2015, 5:50 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 834f69a > src/slave/containerizer/provisioner.cpp PRE-CREATION > 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/flags.hpp e31a418 > src/slave/flags.cpp add4196 > src/tests/containerizer/provisioner_docker_tests.cpp 1b0c304 > > Diff: https://reviews.apache.org/r/38137/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Timothy Chen > >
