----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43127/#review118704 -----------------------------------------------------------
src/slave/containerizer/mesos/provisioner/appc/cache.hpp (line 23) <https://reviews.apache.org/r/43127/#comment180008> we don't include protobuf header directly. Can you include <mesos/mesos.hpp> instead? src/slave/containerizer/mesos/provisioner/appc/cache.hpp (line 25) <https://reviews.apache.org/r/43127/#comment180007> Can you put stout headers above mesos headers. src/slave/containerizer/mesos/provisioner/appc/cache.hpp (line 58) <https://reviews.apache.org/r/43127/#comment180010> Please update the comments. src/slave/containerizer/mesos/provisioner/appc/cache.hpp (lines 62 - 63) <https://reviews.apache.org/r/43127/#comment180016> Hum, I don't like the fact that we do the validation here. I think the validation should be done before it was moved to storeDir/layers. src/slave/containerizer/mesos/provisioner/appc/cache.hpp (lines 83 - 86) <https://reviews.apache.org/r/43127/#comment180011> We try to avoid over commenting. The name of the class and the fields in this class is pretty descriptive. So no need for the verbose comment here. src/slave/containerizer/mesos/provisioner/appc/cache.hpp (lines 89 - 91) <https://reviews.apache.org/r/43127/#comment180012> Kill this as well. src/slave/containerizer/mesos/provisioner/appc/cache.hpp (lines 94 - 96) <https://reviews.apache.org/r/43127/#comment180017> Move the parameters up ``` Key(const std::string& name, const hashmap<std::string, std::string> labels) ``` src/slave/containerizer/mesos/provisioner/appc/cache.hpp (lines 104 - 106) <https://reviews.apache.org/r/43127/#comment180013> Kill this as well. src/slave/containerizer/mesos/provisioner/appc/cache.hpp (line 116) <https://reviews.apache.org/r/43127/#comment180018> Add a new line above. src/slave/containerizer/mesos/provisioner/appc/cache.hpp (line 117) <https://reviews.apache.org/r/43127/#comment180020> `data` is not very descriptive. How about calling it `imageIds` src/slave/containerizer/mesos/provisioner/appc/cache.cpp (line 66) <https://reviews.apache.org/r/43127/#comment180037> s/addCache/adding/ src/slave/containerizer/mesos/provisioner/appc/cache.cpp (line 73) <https://reviews.apache.org/r/43127/#comment180038> typo. Please turn on spell check in your editor. s/Cache// src/slave/containerizer/mesos/provisioner/appc/cache.cpp (line 84) <https://reviews.apache.org/r/43127/#comment180039> As I suggested previously, please do not put validate logics in createEntry since the image should already been validated before it's moved to 'storeDir'. This is actually a pretty important invariant we rely on. Also, no need for the helper 'createEntry' as 'add' suggests that it'll create the entry. src/slave/containerizer/mesos/provisioner/appc/cache.cpp (lines 193 - 203) <https://reviews.apache.org/r/43127/#comment180042> This is problematic. My understanding is that boost::hash_combine is not commutative. You'll have to sort it before calling hash_combine. Or, a better alternative is to use std::map for 'labels' so that it's sorted by default. src/slave/containerizer/mesos/provisioner/appc/cache.cpp (line 200) <https://reviews.apache.org/r/43127/#comment180040> can you just call two hash_combine here? ``` hash_combine(seed, name); hash_combine(seed, value); ``` src/slave/containerizer/mesos/provisioner/appc/store.cpp (line 90) <https://reviews.apache.org/r/43127/#comment180043> s/createCache/cache/ src/slave/containerizer/mesos/provisioner/appc/store.cpp (line 91) <https://reviews.apache.org/r/43127/#comment180044> Kill this line. src/slave/containerizer/mesos/provisioner/appc/store.cpp (line 136) <https://reviews.apache.org/r/43127/#comment180045> typo. src/slave/containerizer/mesos/provisioner/appc/store.cpp (line 187) <https://reviews.apache.org/r/43127/#comment180051> Let need for the helper function here. Please just inline it. src/tests/containerizer/provisioner_appc_tests.cpp (line 222) <https://reviews.apache.org/r/43127/#comment180055> s/getDefaultImage/getTestImage/ src/tests/containerizer/provisioner_appc_tests.cpp (line 224) <https://reviews.apache.org/r/43127/#comment180054> s/imageInfo/appc. src/tests/containerizer/provisioner_appc_tests.cpp (line 230) <https://reviews.apache.org/r/43127/#comment180053> s/versionLabel/version/ Ditto for below - Jie Yu On Feb. 10, 2016, 3:13 p.m., Jojy Varghese wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43127/ > ----------------------------------------------------------- > > (Updated Feb. 10, 2016, 3:13 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 > >
