----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43855/#review120561 -----------------------------------------------------------
First pass. Will do a second pass once the comments below are addressed. Thanks Jojy! src/slave/containerizer/mesos/provisioner/appc/store.cpp (line 115) <https://reviews.apache.org/r/43855/#comment182026> What does 'load' do? src/slave/containerizer/mesos/provisioner/appc/store.cpp (line 130) <https://reviews.apache.org/r/43855/#comment182025> you can do: ``` uriFetcher->share() ``` Also, this fits in one line? src/slave/containerizer/mesos/provisioner/appc/store.cpp (line 197) <https://reviews.apache.org/r/43855/#comment182027> First, we rarely use 'shared_ptr' directly. Second, I think we should be able to avoid this pointer. What you can do is that: 1) `fetchImage` will return `vector<string>` which means: fetch this image and all its dependencies, return me the list of image ids. 2) I think `fetchDependencies` can return `vector<string>` as well, which basically says that fetch the dependencies of this image id, and give me the list of dependent image ids. 3) Similarly, `fetchDependency` will return `vector<string>` as well. This recursion patter sounds more clear to me, instead of passing around the result pointer. src/slave/containerizer/mesos/provisioner/appc/store.cpp (line 199) <https://reviews.apache.org/r/43855/#comment182028> This check sounds unnecessary. src/slave/containerizer/mesos/provisioner/appc/store.cpp (line 203) <https://reviews.apache.org/r/43855/#comment182123> s/rootfsDirs/rootfses/ src/slave/containerizer/mesos/provisioner/appc/store.cpp (line 217) <https://reviews.apache.org/r/43855/#comment182125> Why return 'Path' here. Isn't returning the image id more clear? src/slave/containerizer/mesos/provisioner/appc/store.cpp (line 284) <https://reviews.apache.org/r/43855/#comment182124> I would put it under a staging directory, like we did in docker. ``` <store>/staging/<tmp_XXXX> ``` Also, s/mkdir/staging/ src/slave/containerizer/mesos/provisioner/appc/store.cpp (line 291) <https://reviews.apache.org/r/43855/#comment182126> s/tmpFetchDir/staging/ - Jie Yu On Feb. 23, 2016, 3:30 p.m., Jojy Varghese wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43855/ > ----------------------------------------------------------- > > (Updated Feb. 23, 2016, 3:30 p.m.) > > > Review request for mesos and Jie Yu. > > > Repository: mesos > > > Description > ------- > > This change allows store to fetch an image using Appc image fetcher when an > image is not found in the cache. It also recursively fetches the dependencies > for the image. > > > Diffs > ----- > > src/slave/containerizer/mesos/provisioner/appc/store.cpp > 4b3829175f57fb9aea2478040d96f2f127cbc551 > src/tests/fetcher_cache_tests.cpp e10b3f7ebc21c8c1095564fc40f123087dcf320e > > Diff: https://reviews.apache.org/r/43855/diff/ > > > Testing > ------- > > make check; local image server. > > > Thanks, > > Jojy Varghese > >
