You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@mesos.apache.org by Jojy Varghese <jo...@mesosphere.io> on 2016/02/03 02:31:24 UTC

Review Request 43127: Introduced Appc image cache.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43127/
-----------------------------------------------------------

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 PRE-CREATION 
  src/slave/containerizer/mesos/provisioner/appc/cache.cpp PRE-CREATION 
  src/slave/containerizer/mesos/provisioner/appc/store.cpp 73c4df858a70da3d4cc4a1cb15092165f6ff8fe4 
  src/tests/containerizer/provisioner_appc_tests.cpp e3d08d9e49df93d5290099c8bfd917f60c93e51b 

Diff: https://reviews.apache.org/r/43127/diff/


Testing
-------

make check.


Thanks,

Jojy Varghese

Re: Review Request 43127: Introduced Appc image cache.

Posted by Jie Yu <yu...@gmail.com>.

-----------------------------------------------------------
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
> 
>