-----------------------------------------------------------
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
> 
>

Reply via email to