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




src/slave/containerizer/mesos/provisioner/appc/fetcher.hpp (line 69)
<https://reviews.apache.org/r/43336/#comment180387>

    This can be a file local method, right? No need to declare it in the header.



src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp (line 41)
<https://reviews.apache.org/r/43336/#comment180388>

    We try to avoid global non-POD variables. Can you use char array instead:
    ```
    static const char EXT[] = "aci";
    ...
    ```



src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp (line 89)
<https://reviews.apache.org/r/43336/#comment180389>

    No need for 'doFetchImage' since only this function is using it. Can you 
just inline it here?



src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp (line 93)
<https://reviews.apache.org/r/43336/#comment180390>

    No need for 'const' here as it'll be file local method.



src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp (lines 118 - 123)
<https://reviews.apache.org/r/43336/#comment180391>

    Please use strings::format here



src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp (line 127)
<https://reviews.apache.org/r/43336/#comment180401>

    `s/imageUrl/_url`
    
    s/getFetchUri/getUri/



src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp (line 130)
<https://reviews.apache.org/r/43336/#comment180402>

    s/parseURL/parse/



src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp (line 137)
<https://reviews.apache.org/r/43336/#comment180403>

    s/fetchURL/url/



src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp (lines 148 - 152)
<https://reviews.apache.org/r/43336/#comment180404>

    Move this check up.



src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp (line 185)
<https://reviews.apache.org/r/43336/#comment180393>

    I would
    
    s/imageArchiveFile/aciBundle



src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp (line 189)
<https://reviews.apache.org/r/43336/#comment180395>

    Let's do `[=]() ...`



src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp (line 191)
<https://reviews.apache.org/r/43336/#comment180394>

    I would
    
    `s/tmpImageName/_aciBundle/`



src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp (line 192)
<https://reviews.apache.org/r/43336/#comment180392>

    insert an empty line above



src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp (line 201)
<https://reviews.apache.org/r/43336/#comment180396>

    Can you be consistent and specify the return value here? Also use [=] for 
capture.



src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp (line 204)
<https://reviews.apache.org/r/43336/#comment180397>

    Ditto on [=]



src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp (line 206)
<https://reviews.apache.org/r/43336/#comment180398>

    ```
    const string imagePath = path::join(...);
    
    Try<Nothing> mkdir = os::mkdir(imagePath);
    if (mkdir.isError()) {
      ...
    }
    
    return ...



src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp (lines 207 - 212)
<https://reviews.apache.org/r/43336/#comment180399>

    Hum, do you need to mkdir? will untar create it?



src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp (line 214)
<https://reviews.apache.org/r/43336/#comment180400>

    Do you need to remove aciBundle?
    
    ```
    .then([]() -> Future<Nothing> {
      return os::rm(...);
    });
    ```


- Jie Yu


On Feb. 13, 2016, 1:43 a.m., Jojy Varghese wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43336/
> -----------------------------------------------------------
> 
> (Updated Feb. 13, 2016, 1:43 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-4596
>     https://issues.apache.org/jira/browse/MESOS-4596
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added implementation for simple image discovery for Appc images.
> 
> TODO: Add tests.
> 
> 
> Diffs
> -----
> 
>   src/CMakeLists.txt 9ab84c0898b3adce6063cc50b04ee74cf1471609 
>   src/Makefile.am 5813ab2c33a7de6b612064e894e5f15b5a474e2b 
>   src/slave/containerizer/mesos/provisioner/appc/fetcher.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp PRE-CREATION 
>   src/slave/flags.hpp 20232d645175d0d574c6d896188435277619010d 
>   src/slave/flags.cpp 14ad4dcc0dfb1d7745e58e11e8f66386288395d7 
> 
> Diff: https://reviews.apache.org/r/43336/diff/
> 
> 
> Testing
> -------
> 
> Tested with local http(s) server.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Reply via email to