----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37247/#review97914 -----------------------------------------------------------
I did not realize that we had a combined RR for this chain - please forgive my ignorance for now - I shall review the combined one once you fixed the locally mentioned issues. src/Makefile.am (line 234) <https://reviews.apache.org/r/37247/#comment154020> Lets wrap this to fit, putting the backslash at column 72. ``` DOCKER_PROVISIONER_PROTOS = \ messages/docker_provisioner.pb.cc \ messages/docker_provisioner.pb.h ``` src/Makefile.am (line 402) <https://reviews.apache.org/r/37247/#comment154021> Tabs still off?! src/Makefile.am (line 446) <https://reviews.apache.org/r/37247/#comment154022> Tabs still off?! src/Makefile.am (line 730) <https://reviews.apache.org/r/37247/#comment154023> Tabs still off?! src/Makefile.am (line 758) <https://reviews.apache.org/r/37247/#comment154024> Tabs still off?! src/Makefile.am (line 759) <https://reviews.apache.org/r/37247/#comment154025> Not yours but tab spacing is off, please correct. src/slave/containerizer/provisioners/docker/reference_store.hpp (lines 98 - 99) <https://reviews.apache.org/r/37247/#comment154026> Can we move the process definition into the implementation file to make the headers fully "need to know" only? src/slave/containerizer/provisioners/docker/store.cpp (line 115) <https://reviews.apache.org/r/37247/#comment154030> This seems rather dangerous - you are using the factory method for the initializer here but you ignore the fact that there is indeed chances for it to return a failed try. I see why you went the way it is now as that resulted in very little code. Still, I think we should be a bit more careful here and test for failures. - Till Toenshoff On Aug. 27, 2015, 11:41 p.m., Lily Chen wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/37247/ > ----------------------------------------------------------- > > (Updated Aug. 27, 2015, 11:41 p.m.) > > > Review request for mesos, Till Toenshoff and Timothy Chen. > > > Bugs: MESOS-3021 > https://issues.apache.org/jira/browse/MESOS-3021 > > > Repository: mesos > > > Description > ------- > > Added Docker image reference store. > > > Diffs > ----- > > src/Makefile.am 7b620ff66856b3f0adac121b3297d55ed71a3d99 > src/messages/docker_provisioner.hpp PRE-CREATION > src/messages/docker_provisioner.proto PRE-CREATION > src/slave/containerizer/provisioners/docker/reference_store.hpp > PRE-CREATION > src/slave/containerizer/provisioners/docker/reference_store.cpp > PRE-CREATION > src/slave/containerizer/provisioners/docker/store.hpp PRE-CREATION > src/slave/containerizer/provisioners/docker/store.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/37247/diff/ > > > Testing > ------- > > make check > > Tests will be added in a later review. > > > Thanks, > > Lily Chen > >
