mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jie Yu" <yujie....@gmail.com>
Subject Re: Review Request 37311: Implemented a 'read-only' Appc image store.
Date Tue, 11 Aug 2015 23:21:51 GMT

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



src/slave/containerizer/provisioners/appc/store.hpp (line 39)
<https://reviews.apache.org/r/37311/#comment149753>

    Put '{' in a new line. Please fix all occurances in this file.



src/slave/containerizer/provisioners/appc/store.hpp (lines 39 - 55)
<https://reviews.apache.org/r/37311/#comment149762>

    Can you Move this struct to be a nested struct in Store? (i.e., Store::Image)



src/slave/containerizer/provisioners/appc/store.hpp (line 46)
<https://reviews.apache.org/r/37311/#comment149755>

    Can those be 'const'?



src/slave/containerizer/provisioners/appc/store.hpp (line 60)
<https://reviews.apache.org/r/37311/#comment149756>

    One extra line please.



src/slave/containerizer/provisioners/appc/store.hpp (line 61)
<https://reviews.apache.org/r/37311/#comment149760>

    Could you please add some comments about this class? It's not obvious what this is for.



src/slave/containerizer/provisioners/appc/store.cpp (line 50)
<https://reviews.apache.org/r/37311/#comment149769>

    No need to have a static 'create' function here since this class is in the cpp file. You
can just make the constructor public.



src/slave/containerizer/provisioners/appc/store.cpp (line 60)
<https://reviews.apache.org/r/37311/#comment149770>

    Maybe s/store/root/ and add some comments about that?



src/slave/containerizer/provisioners/appc/store.cpp (line 108)
<https://reviews.apache.org/r/37311/#comment149779>

    Please add a comment explaining that mkdir will return Nothing if images dir already exist.
    
    Do you want to do a os::exists check first to check if flags.appc_store_dir exists?



src/slave/containerizer/provisioners/appc/store.cpp (lines 129 - 134)
<https://reviews.apache.org/r/37311/#comment149793>

    No need to call 'realpath' here every time. We just need to make sure 'root' is a canonicalized
realpath.



src/slave/containerizer/provisioners/appc/store.cpp (line 141)
<https://reviews.apache.org/r/37311/#comment149794>

    s/id/imageId/



src/slave/containerizer/provisioners/appc/store.cpp (lines 141 - 144)
<https://reviews.apache.org/r/37311/#comment149795>

    Hum, let's merge this function into 'recover' for now since it's the only caller.
    
    IN that way, you can get rid of this code because imageId is available already.



src/slave/containerizer/provisioners/appc/store.cpp (line 171)
<https://reviews.apache.org/r/37311/#comment149781>

    `s/images_/result/`



src/slave/containerizer/provisioners/appc/store.cpp (line 183)
<https://reviews.apache.org/r/37311/#comment149789>

    s/entries/imageIds/



src/slave/containerizer/provisioners/appc/store.cpp (line 185)
<https://reviews.apache.org/r/37311/#comment149782>

    "storage entry" is a little confusing to me. How about:
    
    ```
    return Failure(
        "Failed to list all images under '" +
        paths::getImagesDir(root) + "': " +
        entries.error());
    ```



src/slave/containerizer/provisioners/appc/store.cpp (line 188)
<https://reviews.apache.org/r/37311/#comment149790>

    s/entry/imageId



src/tests/containerizer/appc_provisioner_tests.cpp (line 156)
<https://reviews.apache.org/r/37311/#comment149799>

    s/id/imageId



src/tests/containerizer/appc_provisioner_tests.cpp (line 160)
<https://reviews.apache.org/r/37311/#comment149798>

    s/image/imagePath/



src/tests/containerizer/appc_provisioner_tests.cpp (line 169)
<https://reviews.apache.org/r/37311/#comment149797>

    `s/images_/_images`


- Jie Yu


On Aug. 10, 2015, 7:19 p.m., Jiang Yan Xu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37311/
> -----------------------------------------------------------
> 
> (Updated Aug. 10, 2015, 7:19 p.m.)
> 
> 
> Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Timothy Chen.
> 
> 
> Bugs: MESOS-3194
>     https://issues.apache.org/jira/browse/MESOS-3194
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Implemented a 'read-only' Appc image store.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am 942003149b071a322933e2c085d9122903f69713 
>   src/slave/containerizer/provisioners/appc/store.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/appc/store.cpp PRE-CREATION 
>   src/slave/flags.hpp 881d494c06fea5c382d27b357d65c1baf201ae46 
>   src/slave/flags.cpp 82b6cf47af26f0533ff603a67240777e9a9b986e 
>   src/tests/containerizer/appc_provisioner_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/37311/diff/
> 
> 
> Testing
> -------
> 
> make check.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message