You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Till Toenshoff <to...@me.com> on 2015/09/07 17:49:57 UTC
Re: Review Request 37247: Added Docker image reference store.
-----------------------------------------------------------
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
>
>