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 43855: Added Appc fetcher support to store.
Date Thu, 25 Feb 2016 06:55:42 GMT

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



First pass. Will do a second pass once the comments below are addressed. Thanks Jojy!


src/slave/containerizer/mesos/provisioner/appc/store.cpp (line 115)
<https://reviews.apache.org/r/43855/#comment182026>

    What does 'load' do?



src/slave/containerizer/mesos/provisioner/appc/store.cpp (line 130)
<https://reviews.apache.org/r/43855/#comment182025>

    you can do:
    ```
    uriFetcher->share()
    ```
    
    Also, this fits in one line?



src/slave/containerizer/mesos/provisioner/appc/store.cpp (line 197)
<https://reviews.apache.org/r/43855/#comment182027>

    First, we rarely use 'shared_ptr' directly. Second, I think we should be able to avoid
this pointer. What you can do is that:
    
    1) `fetchImage` will return `vector<string>` which means: fetch this image and all
its dependencies, return me the list of image ids.
    
    2) I think `fetchDependencies` can return `vector<string>` as well, which basically
says that fetch the dependencies of this image id, and give me the list of dependent image
ids.
    
    3) Similarly, `fetchDependency` will return `vector<string>` as well.
    
    This recursion patter sounds more clear to me, instead of passing around the result pointer.



src/slave/containerizer/mesos/provisioner/appc/store.cpp (line 199)
<https://reviews.apache.org/r/43855/#comment182028>

    This check sounds unnecessary.



src/slave/containerizer/mesos/provisioner/appc/store.cpp (line 203)
<https://reviews.apache.org/r/43855/#comment182123>

    s/rootfsDirs/rootfses/



src/slave/containerizer/mesos/provisioner/appc/store.cpp (line 217)
<https://reviews.apache.org/r/43855/#comment182125>

    Why return 'Path' here. Isn't returning the image id more clear?



src/slave/containerizer/mesos/provisioner/appc/store.cpp (line 284)
<https://reviews.apache.org/r/43855/#comment182124>

    I would put it under a staging directory, like we did in docker.
    ```
    <store>/staging/<tmp_XXXX>
    ```
    
    Also, s/mkdir/staging/



src/slave/containerizer/mesos/provisioner/appc/store.cpp (line 291)
<https://reviews.apache.org/r/43855/#comment182126>

    s/tmpFetchDir/staging/


- Jie Yu


On Feb. 23, 2016, 3:30 p.m., Jojy Varghese wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43855/
> -----------------------------------------------------------
> 
> (Updated Feb. 23, 2016, 3:30 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This change allows store to fetch an image using Appc image fetcher when an
> image is not found in the cache. It also recursively fetches the dependencies
> for the image.
> 
> 
> Diffs
> -----
> 
>   src/slave/containerizer/mesos/provisioner/appc/store.cpp 4b3829175f57fb9aea2478040d96f2f127cbc551

>   src/tests/fetcher_cache_tests.cpp e10b3f7ebc21c8c1095564fc40f123087dcf320e 
> 
> Diff: https://reviews.apache.org/r/43855/diff/
> 
> 
> Testing
> -------
> 
> make check; local image server.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>


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