----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37197/#review96496 -----------------------------------------------------------
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) <https://reviews.apache.org/r/37197/#comment151886> 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) <https://reviews.apache.org/r/37197/#comment151913> Missing ``` #include <stout/nothing.hpp> ``` src/slave/containerizer/provisioners/docker/store.hpp (line 33) <https://reviews.apache.org/r/37197/#comment151910> Missing ``` #include <process/owned.hpp> ``` src/slave/containerizer/provisioners/docker/store.hpp (line 36) <https://reviews.apache.org/r/37197/#comment151912> Missing ``` #include "slave/containerizer/fetcher.hpp" ``` src/slave/containerizer/provisioners/docker/store.hpp (line 42) <https://reviews.apache.org/r/37197/#comment152006> How about a short descriptive comment on the purpose of this interface? src/slave/containerizer/provisioners/docker/store.hpp (line 54) <https://reviews.apache.org/r/37197/#comment151948> `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) <https://reviews.apache.org/r/37197/#comment152007> 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) <https://reviews.apache.org/r/37197/#comment151889> Insert blank line here please. src/slave/containerizer/provisioners/docker/store.cpp (line 28) <https://reviews.apache.org/r/37197/#comment152031> This should go above the stout headers. src/slave/containerizer/provisioners/docker/store.cpp (line 32) <https://reviews.apache.org/r/37197/#comment151891> This should be the first include. src/slave/containerizer/provisioners/docker/store.cpp (line 34) <https://reviews.apache.org/r/37197/#comment151893> 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: http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Namespaces 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) <https://reviews.apache.org/r/37197/#comment152008> Kill this blank line please. src/slave/containerizer/provisioners/docker/store.cpp (line 61) <https://reviews.apache.org/r/37197/#comment151945> Insert blank line please. src/slave/containerizer/provisioners/docker/store.cpp (line 103) <https://reviews.apache.org/r/37197/#comment151946> Insert blank line please. src/slave/containerizer/provisioners/docker/store.cpp (line 126) <https://reviews.apache.org/r/37197/#comment151916> s/uri/uri schemes are/ ? src/slave/containerizer/provisioners/docker/store.cpp (line 133) <https://reviews.apache.org/r/37197/#comment151902> 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) <https://reviews.apache.org/r/37197/#comment151900> I am not a big fan of such magic numbers (referring to that '7'). How about following the pattern the fetcher is using here (src/slave/containerizer/fetcher.cpp)? ``` [...] 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) <https://reviews.apache.org/r/37197/#comment151903> 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) <https://reviews.apache.org/r/37197/#comment151923> 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) <https://reviews.apache.org/r/37197/#comment151924> 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) <https://reviews.apache.org/r/37197/#comment151925> s/json/JSON/ src/slave/containerizer/provisioners/docker/store.cpp (line 190) <https://reviews.apache.org/r/37197/#comment151926> 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) <https://reviews.apache.org/r/37197/#comment151930> I thought we got rid of `os::basename` and suggest using `Path::basename` instead?! `::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) <https://reviews.apache.org/r/37197/#comment151927> 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) <https://reviews.apache.org/r/37197/#comment151929> How about handling failures here as well? src/slave/containerizer/provisioners/docker/store.cpp (lines 273 - 274) <https://reviews.apache.org/r/37197/#comment151931> 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: " + WSTRINGIFY(s.get().status.get())); } ``` src/slave/containerizer/provisioners/docker/store.cpp (lines 343 - 344) <https://reviews.apache.org/r/37197/#comment151932> See above on the exit/status code mixup. src/slave/containerizer/provisioners/docker/store.cpp (line 352) <https://reviews.apache.org/r/37197/#comment151933> See above on the consistency of your initialize & validate step formatting. src/slave/containerizer/provisioners/docker/store.cpp (line 356) <https://reviews.apache.org/r/37197/#comment151947> Insert blank line please. src/slave/containerizer/provisioners/docker/store.cpp (line 368) <https://reviews.apache.org/r/37197/#comment151934> See above on the use of os::basename vs. Path::basename. src/slave/containerizer/provisioners/docker/store.cpp (line 375) <https://reviews.apache.org/r/37197/#comment152019> 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 > >
