----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43127/#review118911 -----------------------------------------------------------
Fix it, then Ship it! src/slave/containerizer/mesos/provisioner/appc/cache.cpp (line 97) <https://reviews.apache.org/r/43127/#comment180221> Indentation. src/slave/containerizer/mesos/provisioner/appc/cache.cpp (line 101) <https://reviews.apache.org/r/43127/#comment180222> Do you need this temp variable? src/slave/containerizer/mesos/provisioner/appc/store.cpp (line 51) <https://reviews.apache.org/r/43127/#comment180223> Hum, copying the cache sounds weired to me since we get rid of the shared_ptr. Can we use Owned<Cache> here instead? That means you need to return Try<Owned<Cache>> for Cache::create. src/slave/containerizer/mesos/provisioner/appc/store.cpp (line 148) <https://reviews.apache.org/r/43127/#comment180226> This should be Option? src/slave/containerizer/mesos/provisioner/appc/store.cpp (lines 156 - 179) <https://reviews.apache.org/r/43127/#comment180227> I guess this is fine for now. The logics will change soon as we're adding fetching support. I think what we should do here in the future is: ``` 1. get imageId as you did 2. if imageId is not found, call fetch using 'appc', move it to store and goto 4 3. if imageId is found, check if it's in the storeDir (cache.get(imageId)) 4. If yes, fetch its dependencies 5. If not, goto 2 ``` The validation will be in step 2 after a layer is downloaded. src/slave/containerizer/mesos/provisioner/appc/store.cpp (line 168) <https://reviews.apache.org/r/43127/#comment180225> No need for this temp var. src/slave/containerizer/mesos/provisioner/appc/store.cpp (line 177) <https://reviews.apache.org/r/43127/#comment180224> No need for this temp var. - Jie Yu On Feb. 11, 2016, 4:15 p.m., Jojy Varghese wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43127/ > ----------------------------------------------------------- > > (Updated Feb. 11, 2016, 4:15 p.m.) > > > Review request for mesos and Jie Yu. > > > Bugs: MESOS-4439 and MESOS-4575 > https://issues.apache.org/jira/browse/MESOS-4439 > https://issues.apache.org/jira/browse/MESOS-4575 > > > Repository: mesos > > > Description > ------- > > Image cache will cache metadata about all Appc images stores in the store's > image directory. This cache could be useful to: > - Quickly query if an image is present in the store. > - By sharing the cache with other components like fetcher, an image can be > checked if its present before fetching it. > > > Diffs > ----- > > src/slave/containerizer/mesos/provisioner/appc/cache.hpp > 4a63d479d3328605bac08fddffe190dbe21ad2b7 > src/slave/containerizer/mesos/provisioner/appc/cache.cpp > af69db3cdfea05c72ecc6ed18adc9ce520ecdd7e > src/slave/containerizer/mesos/provisioner/appc/store.cpp > 1f9b573f9388aafff3304358b8822a48075afb44 > src/tests/containerizer/provisioner_appc_tests.cpp > 012dba4e24b9a94dc8da0d329baf4bec2d33ffca > > Diff: https://reviews.apache.org/r/43127/diff/ > > > Testing > ------- > > make check. > > > Thanks, > > Jojy Varghese > >
