mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jojy Varghese" <j...@mesosphere.io>
Subject Re: Review Request 38137: Added Docker provisioner and local store
Date Tue, 08 Sep 2015 18:35:09 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38137/#review98030
-----------------------------------------------------------



src/slave/containerizer/provisioner.hpp (line 54)
<https://reviews.apache.org/r/38137/#comment154200>

    doxygen style?



src/slave/containerizer/provisioner.cpp (line 36)
<https://reviews.apache.org/r/38137/#comment154181>

    I think we should guard against multiple "create" calls. Call to "create" multiple times
here would repeat the whole logic of creating the creator map and provsioners.



src/slave/containerizer/provisioners/docker.hpp (line 56)
<https://reviews.apache.org/r/38137/#comment154202>

    doxygen comments? This applies for other places too.



src/slave/containerizer/provisioners/docker.hpp (line 65)
<https://reviews.apache.org/r/38137/#comment154186>

    s/repo/repositorypath



src/slave/containerizer/provisioners/docker.hpp (line 67)
<https://reviews.apache.org/r/38137/#comment154185>

    I believe the default value of an option is None.So need not explicitly declare.



src/slave/containerizer/provisioners/docker.hpp (line 94)
<https://reviews.apache.org/r/38137/#comment154187>

    Is it layers or just layer ids? If its just layer ids, then the struct should be DockerImageInfo
(and not DockerImage).



src/slave/containerizer/provisioners/docker.hpp (line 121)
<https://reviews.apache.org/r/38137/#comment154188>

    According to google style guide, this should be declared last. 
    
    Also, why not use the "delete" keyword (c++11)?



src/slave/containerizer/provisioners/docker.cpp (line 37)
<https://reviews.apache.org/r/38137/#comment154192>

    maybe explicit namespace names?



src/slave/containerizer/provisioners/docker.cpp (line 56)
<https://reviews.apache.org/r/38137/#comment154191>

    I strongly believe that we should have strong/explicit ownership semantics for pointers
(instead of raw pointers).



src/slave/containerizer/provisioners/docker.cpp (line 89)
<https://reviews.apache.org/r/38137/#comment154194>

    Would be better if we follow the factory patttern (create method that returns a Try)?



src/slave/containerizer/provisioners/docker.cpp (line 111)
<https://reviews.apache.org/r/38137/#comment154196>

    should we create a guard here so that multiple calls to "create" wont call multiple "process"
create?



src/slave/containerizer/provisioners/docker.cpp (line 172)
<https://reviews.apache.org/r/38137/#comment154204>

    should we check/validate for the docker_rootfs_dir?



src/slave/containerizer/provisioners/docker.cpp (line 183)
<https://reviews.apache.org/r/38137/#comment154205>

    const?



src/slave/containerizer/provisioners/docker.cpp (line 279)
<https://reviews.apache.org/r/38137/#comment154207>

    since destroy can be called from multiple thread contexts, should we proetct it?



src/slave/containerizer/provisioners/docker/local_store.hpp (line 34)
<https://reviews.apache.org/r/38137/#comment154198>

    destructor declaration should be after constructor(create)?



src/slave/containerizer/provisioners/docker/local_store.hpp (line 53)
<https://reviews.apache.org/r/38137/#comment154208>

    This should be  declared last. Applies everywhere.



src/slave/containerizer/provisioners/docker/local_store.cpp (line 94)
<https://reviews.apache.org/r/38137/#comment154213>

    s/refStore/referenceStore?



src/slave/containerizer/provisioners/docker/local_store.cpp (line 102)
<https://reviews.apache.org/r/38137/#comment154211>

    guard for multiple calls?



src/slave/containerizer/provisioners/docker/local_store.cpp (line 157)
<https://reviews.apache.org/r/38137/#comment154212>

    here it is possible that refStore could be not created and we still have a LocalStoreProcess
object. Maybe we should move the refStore creation to before line 152.



src/slave/containerizer/provisioners/docker/local_store.cpp (line 167)
<https://reviews.apache.org/r/38137/#comment154215>

    const?



src/slave/containerizer/provisioners/docker/local_store.cpp (line 174)
<https://reviews.apache.org/r/38137/#comment154220>

    Can this return an error?



src/slave/containerizer/provisioners/docker/local_store.cpp (line 179)
<https://reviews.apache.org/r/38137/#comment154221>

    Can this return an error?



src/slave/containerizer/provisioners/docker/local_store.cpp (line 280)
<https://reviews.apache.org/r/38137/#comment154226>

    s/layers/layerIds ?



src/slave/flags.hpp (line 56)
<https://reviews.apache.org/r/38137/#comment154193>

    Adding more fields to base Flag class might not scale? Should we adopt feature/subsystem
specific flags as subclass of base Flags?


- Jojy Varghese


On Sept. 7, 2015, 11:31 p.m., Timothy Chen wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38137/
> -----------------------------------------------------------
> 
> (Updated Sept. 7, 2015, 11:31 p.m.)
> 
> 
> Review request for mesos, 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 5fdca0f 
>   src/messages/docker_provisioner.hpp PRE-CREATION 
>   src/messages/docker_provisioner.proto PRE-CREATION 
>   src/slave/containerizer/provisioner.hpp 9e0e0b8 
>   src/slave/containerizer/provisioner.cpp 95894c0 
>   src/slave/containerizer/provisioners/appc.cpp fc5ee19 
>   src/slave/containerizer/provisioners/appc/paths.hpp fb3a1a7 
>   src/slave/containerizer/provisioners/appc/paths.cpp e6be851 
>   src/slave/containerizer/provisioners/docker.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker.cpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/local_store.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/local_store.cpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/paths.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/paths.cpp 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/paths.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/paths.cpp PRE-CREATION 
>   src/slave/flags.hpp b8335aa 
>   src/slave/flags.cpp 7539441 
>   src/tests/containerizer/docker_provisioner_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/38137/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message