----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43336/#review118947 -----------------------------------------------------------
First pass. Will do a more detailed path tomorrow. Thanks Jojy! src/slave/containerizer/mesos/provisioner/appc/fetcher.hpp (lines 30 - 36) <https://reviews.apache.org/r/43336/#comment180254> Why do you need this? Can you just include <mesos/uri/fetcher.hpp> ? src/slave/containerizer/mesos/provisioner/appc/fetcher.hpp (lines 55 - 56) <https://reviews.apache.org/r/43336/#comment180257> Swap the order of these two parameters. We usually put flags first. Also, please use `const Shared<uri::Fetcher>& fetcher`. (You can share 'Shared' pointers). src/slave/containerizer/mesos/provisioner/appc/fetcher.hpp (lines 68 - 70) <https://reviews.apache.org/r/43336/#comment180263> Please remove the 'const' in the end. We try to avoid doing that unless it's necessary. Especially, in this case, 'fetch' does not absolutely indicate that it's const function (depending on the internal impl.) src/slave/containerizer/mesos/provisioner/appc/fetcher.hpp (lines 73 - 74) <https://reviews.apache.org/r/43336/#comment180259> Please fix the indentation for parameters. src/slave/containerizer/mesos/provisioner/appc/fetcher.hpp (line 74) <https://reviews.apache.org/r/43336/#comment180258> NO need for mesos:: namespace prefix since you're in mesos namespace already. src/slave/containerizer/mesos/provisioner/appc/fetcher.hpp (lines 76 - 86) <https://reviews.apache.org/r/43336/#comment180264> Ditto on removing const in the end. src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp (line 29) <https://reviews.apache.org/r/43336/#comment180265> Add a new line above. src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp (lines 53 - 54) <https://reviews.apache.org/r/43336/#comment180267> Why use leading underscore for parameters here? src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp (line 83) <https://reviews.apache.org/r/43336/#comment180269> No need to create this temp variable. Compiler will do it anyway. User appc.name() in all other places sounds pretty clean to me. src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp (line 88) <https://reviews.apache.org/r/43336/#comment180270> s/discoveryUrl/url/ src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp (line 95) <https://reviews.apache.org/r/43336/#comment180271> I would just inline this helper here. We try to avoid unnecessary helpers because it hurts readability (readers have to jump around in the code). src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp (lines 148 - 189) <https://reviews.apache.org/r/43336/#comment180275> Hum, I find this unncessary and overly complicated. Can you just assume each time we call 'fetch', the 'directory' is an empty temp directory. src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp (line 177) <https://reviews.apache.org/r/43336/#comment180276> This is not necessary, the caller will cleanup the 'directory'. src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp (line 508) <https://reviews.apache.org/r/43336/#comment180277> I would split fetch and untar into two steps. At the top level, I would like to see something like: ``` return fetch(...) .then(lambda::bind(&untar, ...)) .then(lambda::bind(&move, ...)) .onAny(lambda::bind(&finish, ...)) ``` src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp (lines 523 - 527) <https://reviews.apache.org/r/43336/#comment180278> Hum, can you move this to the lambda in onAny? - Jie Yu On Feb. 11, 2016, 5:52 p.m., Jojy Varghese wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43336/ > ----------------------------------------------------------- > > (Updated Feb. 11, 2016, 5:52 p.m.) > > > Review request for mesos and Jie Yu. > > > Bugs: MESOS-4596 > https://issues.apache.org/jira/browse/MESOS-4596 > > > Repository: mesos > > > Description > ------- > > Added implementation for simple image discovery for Appc images. > > TODO: Add tests. > > > Diffs > ----- > > src/CMakeLists.txt 4a2954498efa48a4eb43f82827ff1d6f5f65d389 > src/Makefile.am 22f51319a1c06da02cac5822d42315e3027cf500 > src/slave/containerizer/mesos/provisioner/appc/fetcher.hpp PRE-CREATION > src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp PRE-CREATION > src/slave/flags.hpp 23ec158f4a95fa76d657d9eb6a4a6fe30d57e5b6 > src/slave/flags.cpp 24a23325cc255d0d7b7af7ed096b6d3012ad75c7 > > Diff: https://reviews.apache.org/r/43336/diff/ > > > Testing > ------- > > Tested with local http(s) server. > > > Thanks, > > Jojy Varghese > >
