This is an automatically generated e-mail. To reply, visit:

Great work Lily, very clean considering that this is your first bigger 
contribution so far (hope I did not miss something here :) ). I really like the 
way things are structured, elegantly scaling.

I am puzzled on how you were able to get this compiled as you are using 
"os::basename" which got removed from stout a while ago and I am assuming that 
you dont bring it back later in this chain. I may have omitted vital 
explanations in some of the issues I raised - please feel free to ask for 
clarification / reasoning where needed.

Once you addressed these issues, I would like to give the chain another pass, 
hence I will add myself as a reviewer, hoping you are OK with that.

src/Makefile.am (line 745)

    Formatting looks off in reviewboard -- please check if you are using 
hard-tabs (displayed as 8 chars).

src/slave/containerizer/provisioners/docker/store.hpp (line 29)

    #include <stout/nothing.hpp>

src/slave/containerizer/provisioners/docker/store.hpp (line 33)

    #include <process/owned.hpp>

src/slave/containerizer/provisioners/docker/store.hpp (line 36)

    #include "slave/containerizer/fetcher.hpp"

src/slave/containerizer/provisioners/docker/store.hpp (line 42)

    How about a short descriptive comment on the purpose of this interface?

src/slave/containerizer/provisioners/docker/store.hpp (line 54)

    `DockerImage` is not defined by this RR but by 37198. We usually go with 
one of those two options here;
    A. mark the entire chain as "must get committed entirely"
    B. make sure each RR is committable without breaking build (or function)

src/slave/containerizer/provisioners/docker/store.hpp (line 61)

    Consider adding a protected default constructor to underline the fact that 
this is a pure virtual interface.

src/slave/containerizer/provisioners/docker/store.hpp (line 91)

    Insert blank line here please.

src/slave/containerizer/provisioners/docker/store.cpp (line 28)

    This should go above the stout headers.

src/slave/containerizer/provisioners/docker/store.cpp (line 32)

    This should be the first include.

src/slave/containerizer/provisioners/docker/store.cpp (line 34)

    We should avoid such general "using namespace xxx" as it pollutes our local 
namespace. Prefer explicit references as in "using process::Process" as also 
described in the google styleguide:
    However, I did not raise an issue here because our codebase shows many uses 
of "using namespace process;", so maybe we actually want to trump the google 
styleguide in specific cases? I vaguely remember we had a discussion around 
this a while back but can not find the results right now -- maybe others can 
chime in here?

src/slave/containerizer/provisioners/docker/store.cpp (line 46)

    Kill this blank line please.

src/slave/containerizer/provisioners/docker/store.cpp (line 61)

    Insert blank line please.

src/slave/containerizer/provisioners/docker/store.cpp (line 103)

    Insert blank line please.

src/slave/containerizer/provisioners/docker/store.cpp (line 126)

    s/uri/uri schemes are/ ?

src/slave/containerizer/provisioners/docker/store.cpp (line 133)

    How about `imagePath` or even `path`? instead of `imageUri` as you are 
effectively trimming it from a URI to a local path?

src/slave/containerizer/provisioners/docker/store.cpp (line 135)

    I am not a big fan of such magic numbers (referring to that '7'). How about 
following the pattern the fetcher is using here 
    namespace slave {
    static const string FILE_URI_PREFIX = "file://";
    if (strings::startsWith(imageUri, FILE_URI_PREFIX)) {
        imageUri = imageUri.substr(FILE_URI_PREFIX.size());

src/slave/containerizer/provisioners/docker/store.cpp (line 140)

    I think it is more straightforward if we removed the word "uri" from all 
the below error messages -- that would be following the argument I made above.
    Also there is a space missing before `imageUri`.

src/slave/containerizer/provisioners/docker/store.cpp (line 146)

    Not sure if this comment adds any value as is. I think we can safely remove 
it - also considering that it currently does not head the actual reading part 
which starts at the next block.

src/slave/containerizer/provisioners/docker/store.cpp (line 177)

    We rarely use such double blank lines when the scope does not change - in 
this case I guess it was not intentional?

src/slave/containerizer/provisioners/docker/store.cpp (line 187)


src/slave/containerizer/provisioners/docker/store.cpp (line 190)

    Again following my initial argument, this is not a URI anymore, it is a 
path - shall we call it
    `layerPath` instead?

src/slave/containerizer/provisioners/docker/store.cpp (line 211)

    I thought we got rid of `os::basename` and suggest using `Path::basename` 
    `::basename` (which os::basename was relying on) is not thread-safe on some 
systems, please avoid it.

src/slave/containerizer/provisioners/docker/store.cpp (line 226)

    I am unsure if users will understand this log line -- do you think a 
regular mesos user will benefit from this information? Maybe a VLOG would be 
more appropriate?

src/slave/containerizer/provisioners/docker/store.cpp (line 242)

    How about handling failures here as well?

src/slave/containerizer/provisioners/docker/store.cpp (lines 273 - 274)

    You are actually displaying the waitpid status, not the exit code and that 
may confuse people. To get the exit code out of the waitpid status, the 
following snippet could be used:
      // The status is a waitpid-result which has to be checked for SIGNAL
      // based termination before masking out the exit-code.
      if (!WIFEXITED(s.get().status.get()) || WEXITSTATUS(s.get().status.get()) 
!= 0) {
        return Failure("Untar failed with exit code: " + 

src/slave/containerizer/provisioners/docker/store.cpp (lines 343 - 344)

    See above on the exit/status code mixup.

src/slave/containerizer/provisioners/docker/store.cpp (line 352)

    See above on the consistency of your initialize & validate step formatting.

src/slave/containerizer/provisioners/docker/store.cpp (line 356)

    Insert blank line please.

src/slave/containerizer/provisioners/docker/store.cpp (line 368)

    See above on the use of os::basename vs. Path::basename.

src/slave/containerizer/provisioners/docker/store.cpp (line 375)

    s/if(/if (/

- Till Toenshoff

On Aug. 25, 2015, 8:57 p.m., Lily Chen wrote:
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37197/
> -----------------------------------------------------------
> (Updated Aug. 25, 2015, 8:57 p.m.)
> Review request for mesos, Ian Downes, Jie Yu, Jojy Varghese, and Timothy Chen.
> Bugs: MESOS-2849
>     https://issues.apache.org/jira/browse/MESOS-2849
> Repository: mesos
> Description
> -------
> Stored images currently kept indefinitely.
> Diffs
> -----
>   src/Makefile.am 571e1ac0f96b2452797a478680b540f2aab63aab 
>   src/slave/containerizer/provisioners/docker/store.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/store.cpp PRE-CREATION 
>   src/slave/flags.hpp e56738e2dfd6593ef8f093687919da287af78f77 
>   src/slave/flags.cpp b36710d6d7a7250bc071a57310a2d54bfb3bc624 
> Diff: https://reviews.apache.org/r/37197/diff/
> Testing
> -------
> make check
> Thanks,
> Lily Chen

Reply via email to