----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43336/#review118430 -----------------------------------------------------------
This is a partial review. I scanned the entire patch and realized that the semantics of Fetcher is wiered to me. IIUC, the Fetcher::fetch interface takes an 'Image::Appc' and a 'directory' and it'll download all needed layers into 'directory'. Now, I am thinking about what will the appc::Store::get() be look like? It'll have to call 'Fetcher::fetcher' no matter if the name+labels is found or not in the cache because even if it's found in the cache, some layers might still be missing due to potential cache eviction of layers. Then, it sounds like Fetcher::fetch is doing what Store is supposed to be doing. I am wondering if we should just do the actual fetching in Fetcher and put the rest of the logics in appc::Store. In other words, Fetcher::fetch will just download a single layer associated with 'Image::Appc', untar it, validate it (optional). The Store will be responsible for reading its dependencies and call Fetcher::fetch again for those dependencies. In that way, Cache will be internal to Store and you don't have to do explicit synchronization. Based on that, appc::Fetcher can actually be a plugin under uri::Fetcher. Happy to chat more if you have any question. Thanks Jojy! src/slave/containerizer/mesos/provisioner/appc/fetcher.hpp (line 24) <https://reviews.apache.org/r/43336/#comment179641> This should be ``` #include "slave/flags.hpp" ``` src/slave/containerizer/mesos/provisioner/appc/fetcher.hpp (line 48) <https://reviews.apache.org/r/43336/#comment179656> I think we should inject uri::Fetcher here. ``` static Try<Owned<Fetcher>> create( const Flags& flags, const Cache& cache, const Shared<uri::Fetcher>& fetcher) ``` src/slave/containerizer/mesos/provisioner/appc/fetcher.hpp (line 67) <https://reviews.apache.org/r/43336/#comment179642> const Owned<T>& does not make sense. Please use: ``` Fetcher(Owned<FetcherProcess> process) ``` src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp (lines 57 - 59) <https://reviews.apache.org/r/43336/#comment179643> No need for the 'create' function here. validations should be done in Fetcher::create and Fetcher::create should be able to call the constructor directly (making the contructor public is OK since it's in a cpp file). src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp (line 59) <https://reviews.apache.org/r/43336/#comment179644> Why Option<Flags> here? Please just use `Flags` here. src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp (lines 65 - 66) <https://reviews.apache.org/r/43336/#comment179649> Can you swap the order of these two parameters? src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp (line 67) <https://reviews.apache.org/r/43336/#comment179648> Owned<uri::Fetcher> does not sound right since there will be multiple entities in the agent sharing the same uri::Fetche in the future. There're two options here: 1) make uri::Fetcher copyable 2) use Shared<uri::Fetcher> I think 2) is easier currently (you need to mark uri::Fetcher::fetch as a const function). src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp (lines 87 - 88) <https://reviews.apache.org/r/43336/#comment179650> Ditto on swapping the order of these two. src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp (lines 127 - 129) <https://reviews.apache.org/r/43336/#comment179658> Why we need this? 'flags.appc_simple_discovery_uri_prefix' already has a default. src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp (lines 127 - 141) <https://reviews.apache.org/r/43336/#comment179660> As I mentioned earlier, this should be moved to Fetcher::create (and kill FetcherProcess::create). src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp (lines 137 - 141) <https://reviews.apache.org/r/43336/#comment179659> OK, this should be injected from top level. src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp (lines 171 - 176) <https://reviews.apache.org/r/43336/#comment179663> I'd prefer wrapping comments in 70 char width (for better readability). src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp (lines 181 - 183) <https://reviews.apache.org/r/43336/#comment179668> This looks wierd to me. do simple discovery on Error? I think cache.find should return an Option so that the logic here makes more sense (if not found in the cache, do simple discovery). src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp (line 187) <https://reviews.apache.org/r/43336/#comment179676> I don't see a reason why spec::getDependencies needs to return an Future. It's just reading a file, right? Returning a Try should be sufficient. Also, I don't like 'getDependencies'. Let's just do spec::getManifest and passing the appc::Manifest instead. src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp (line 216) <https://reviews.apache.org/r/43336/#comment179681> This is very misleading. `doSimpleImageDiscovery` will actually try to fetch the image! src/slave/flags.hpp (line 53) <https://reviews.apache.org/r/43336/#comment179639> s/appc_fetcher_uri_prefix/appc_simple_discovery_uri_prefix/ - Jie Yu On Feb. 8, 2016, 8:30 p.m., Jojy Varghese wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43336/ > ----------------------------------------------------------- > > (Updated Feb. 8, 2016, 8:30 p.m.) > > > Review request for mesos and Jie Yu. > > > 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 > >
