----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37311/#review95005 -----------------------------------------------------------
src/slave/containerizer/provisioners/appc/store.hpp (line 39) <https://reviews.apache.org/r/37311/#comment149753> Put '{' in a new line. Please fix all occurances in this file. src/slave/containerizer/provisioners/appc/store.hpp (lines 39 - 55) <https://reviews.apache.org/r/37311/#comment149762> Can you Move this struct to be a nested struct in Store? (i.e., Store::Image) src/slave/containerizer/provisioners/appc/store.hpp (line 46) <https://reviews.apache.org/r/37311/#comment149755> Can those be 'const'? src/slave/containerizer/provisioners/appc/store.hpp (line 60) <https://reviews.apache.org/r/37311/#comment149756> One extra line please. src/slave/containerizer/provisioners/appc/store.hpp (line 61) <https://reviews.apache.org/r/37311/#comment149760> Could you please add some comments about this class? It's not obvious what this is for. src/slave/containerizer/provisioners/appc/store.cpp (line 50) <https://reviews.apache.org/r/37311/#comment149769> No need to have a static 'create' function here since this class is in the cpp file. You can just make the constructor public. src/slave/containerizer/provisioners/appc/store.cpp (line 60) <https://reviews.apache.org/r/37311/#comment149770> Maybe s/store/root/ and add some comments about that? src/slave/containerizer/provisioners/appc/store.cpp (line 108) <https://reviews.apache.org/r/37311/#comment149779> Please add a comment explaining that mkdir will return Nothing if images dir already exist. Do you want to do a os::exists check first to check if flags.appc_store_dir exists? src/slave/containerizer/provisioners/appc/store.cpp (lines 129 - 134) <https://reviews.apache.org/r/37311/#comment149793> No need to call 'realpath' here every time. We just need to make sure 'root' is a canonicalized realpath. src/slave/containerizer/provisioners/appc/store.cpp (line 141) <https://reviews.apache.org/r/37311/#comment149794> s/id/imageId/ src/slave/containerizer/provisioners/appc/store.cpp (lines 141 - 144) <https://reviews.apache.org/r/37311/#comment149795> Hum, let's merge this function into 'recover' for now since it's the only caller. IN that way, you can get rid of this code because imageId is available already. src/slave/containerizer/provisioners/appc/store.cpp (line 171) <https://reviews.apache.org/r/37311/#comment149781> `s/images_/result/` src/slave/containerizer/provisioners/appc/store.cpp (line 183) <https://reviews.apache.org/r/37311/#comment149789> s/entries/imageIds/ src/slave/containerizer/provisioners/appc/store.cpp (line 185) <https://reviews.apache.org/r/37311/#comment149782> "storage entry" is a little confusing to me. How about: ``` return Failure( "Failed to list all images under '" + paths::getImagesDir(root) + "': " + entries.error()); ``` src/slave/containerizer/provisioners/appc/store.cpp (line 188) <https://reviews.apache.org/r/37311/#comment149790> s/entry/imageId src/tests/containerizer/appc_provisioner_tests.cpp (line 156) <https://reviews.apache.org/r/37311/#comment149799> s/id/imageId src/tests/containerizer/appc_provisioner_tests.cpp (line 160) <https://reviews.apache.org/r/37311/#comment149798> s/image/imagePath/ src/tests/containerizer/appc_provisioner_tests.cpp (line 169) <https://reviews.apache.org/r/37311/#comment149797> `s/images_/_images` - Jie Yu On Aug. 10, 2015, 7:19 p.m., Jiang Yan Xu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/37311/ > ----------------------------------------------------------- > > (Updated Aug. 10, 2015, 7:19 p.m.) > > > Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Timothy Chen. > > > Bugs: MESOS-3194 > https://issues.apache.org/jira/browse/MESOS-3194 > > > Repository: mesos > > > Description > ------- > > Implemented a 'read-only' Appc image store. > > > Diffs > ----- > > src/Makefile.am 942003149b071a322933e2c085d9122903f69713 > src/slave/containerizer/provisioners/appc/store.hpp PRE-CREATION > src/slave/containerizer/provisioners/appc/store.cpp PRE-CREATION > src/slave/flags.hpp 881d494c06fea5c382d27b357d65c1baf201ae46 > src/slave/flags.cpp 82b6cf47af26f0533ff603a67240777e9a9b986e > src/tests/containerizer/appc_provisioner_tests.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/37311/diff/ > > > Testing > ------- > > make check. > > > Thanks, > > Jiang Yan Xu > >
