----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37247/#review94646 -----------------------------------------------------------
include/mesos/mesos.proto (line 1397) <https://reviews.apache.org/r/37247/#comment149238> Since this is not a protobuf message that is going to be used in the public API, we usually put these in the src folder, look at messages.proto and where it is at and I think we should create a new proto file just for docker provisioner. include/mesos/mesos.proto (line 1404) <https://reviews.apache.org/r/37247/#comment149239> This is already in the image message, we just need to name this "name" include/mesos/mesos.proto (line 1405) <https://reviews.apache.org/r/37247/#comment149240> layer_ids src/slave/containerizer/provisioners/docker/reference_store.hpp (line 46) <https://reviews.apache.org/r/37247/#comment149241> Don't we persist to disk? src/slave/containerizer/provisioners/docker/reference_store.hpp (line 57) <https://reviews.apache.org/r/37247/#comment149242> don't we load from disk? why would we need to recover if it's already in memory? src/slave/containerizer/provisioners/docker/reference_store.hpp (line 66) <https://reviews.apache.org/r/37247/#comment149243> I think we are converting comments to javadoc style now, so you'll need to also comment on the params too. src/slave/containerizer/provisioners/docker/reference_store.hpp (line 69) <https://reviews.apache.org/r/37247/#comment149244> What does available mean here? src/slave/containerizer/provisioners/docker/reference_store.hpp (line 79) <https://reviews.apache.org/r/37247/#comment149245> two spaces between classes src/slave/containerizer/provisioners/docker/reference_store.hpp (line 101) <https://reviews.apache.org/r/37247/#comment149246> How about just say Write out referencen store state to persistent store. How about just calling this persist? Save doesn't seem to convey what this method does. src/slave/containerizer/provisioners/docker/reference_store.cpp (line 28) <https://reviews.apache.org/r/37247/#comment149247> Move this above stout src/slave/containerizer/provisioners/docker/reference_store.cpp (line 127) <https://reviews.apache.org/r/37247/#comment149248> Why create a heap allocated object? Why not just: DockerProvisionerImages images; ? src/slave/containerizer/provisioners/docker/reference_store.cpp (line 137) <https://reviews.apache.org/r/37247/#comment149249> Space between foreach and ( src/slave/containerizer/provisioners/docker/reference_store.cpp (line 162) <https://reviews.apache.org/r/37247/#comment149250> Space between if and ( src/slave/containerizer/provisioners/docker/reference_store.cpp (line 163) <https://reviews.apache.org/r/37247/#comment149251> LOG something here src/slave/containerizer/provisioners/docker/reference_store.cpp (line 186) <https://reviews.apache.org/r/37247/#comment149252> Why missing layer means we don't load up the image? And when would this happen? We should definitely log here too! src/slave/containerizer/provisioners/docker/reference_store.cpp (line 188) <https://reviews.apache.org/r/37247/#comment149253> I think we should add VLOG(1) or VLOG(2) of logging all the images we loaded up. - Timothy Chen On Aug. 8, 2015, 1:38 a.m., Lily Chen wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/37247/ > ----------------------------------------------------------- > > (Updated Aug. 8, 2015, 1:38 a.m.) > > > Review request for mesos and Timothy Chen. > > > Bugs: MESOS-3021 > https://issues.apache.org/jira/browse/MESOS-3021 > > > Repository: mesos > > > Description > ------- > > Added Docker image reference store. > > > Diffs > ----- > > include/mesos/mesos.proto 80f56ac2178b24ff19f57c1ace13f567843c7807 > src/Makefile.am c213ac779e7acc3235312ca9524b3959417b8c33 > src/slave/containerizer/provisioners/docker/reference_store.hpp > PRE-CREATION > src/slave/containerizer/provisioners/docker/reference_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 > >
