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 68454: Windows: Fetch blobs with V2S2 Docker image manifest.
Date Fri, 24 Aug 2018 05:45:19 GMT

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




src/uri/fetchers/docker.cpp
Lines 99 (patched)
<https://reviews.apache.org/r/68454/#comment291392>

    Can you explain a bit why you need to do this?
    
    this sounds to me that we need to make `URI` streaming function to be aware windows, instead
of doing it adhoc here?



src/uri/fetchers/docker.cpp
Lines 231-232 (patched)
<https://reviews.apache.org/r/68454/#comment291393>

    Ditto.



src/uri/fetchers/docker.cpp
Lines 235 (patched)
<https://reviews.apache.org/r/68454/#comment291394>

    Can you check with Andy. Let's be consisent on doing encoding for path on windows. If
we use percentage encoding in other places, let's be consistent



src/uri/fetchers/docker.cpp
Lines 357-358 (patched)
<https://reviews.apache.org/r/68454/#comment291395>

    This is super confusing to readers.
    
    I would suggest we just leave one `download` overload, and kill the others.
    
    Instead, introduce a helper to get blobPath:
    ```
    static string getBlobPath(
        const string& directory,
        const URI& uri) {
    #ifdef __WINDOWS__
       ...
    #else
       ...
    #endif
    }
    ```



src/uri/fetchers/docker.cpp
Lines 712 (patched)
<https://reviews.apache.org/r/68454/#comment291385>

    nits: one blank line above



src/uri/fetchers/docker.cpp
Lines 728 (patched)
<https://reviews.apache.org/r/68454/#comment291386>

    since this is `.then`, the argument to the lambda should be `http::Response` instead of
`Future<http::Response>`
    
    ```
    .then(defer(self(), [=](const http::Response& future)
    ```



src/uri/fetchers/docker.cpp
Lines 735-736 (patched)
<https://reviews.apache.org/r/68454/#comment291387>

    Any reason we don't return a `Failure` here?



src/uri/fetchers/docker.cpp
Lines 925-926 (patched)
<https://reviews.apache.org/r/68454/#comment291389>

    I got confused. Why this is related to manifest? Please follow what I suggested in the
last review.



src/uri/fetchers/docker.cpp
Lines 969 (patched)
<https://reviews.apache.org/r/68454/#comment291390>

    No need for the warning. The caller should be responsible for printing



src/uri/fetchers/docker.cpp
Lines 987 (patched)
<https://reviews.apache.org/r/68454/#comment291388>

    Looks like this is never used?



src/uri/fetchers/docker.cpp
Lines 1004 (patched)
<https://reviews.apache.org/r/68454/#comment291396>

    we typically avoid too deep nesting by short circuit first:
    ```
    for (int i = 0; i < manifest->layers_size(); i++) {
      if (blobsum != manifest->layers(i).digest()) {
        continue;
      }
      
      for (...) {
      }
      
      break;
    }
    ```



src/uri/fetchers/docker.cpp
Lines 1005 (patched)
<https://reviews.apache.org/r/68454/#comment291397>

    Please do j++ to be consistent with i++ above.



src/uri/fetchers/docker.cpp
Lines 1026 (patched)
<https://reviews.apache.org/r/68454/#comment291391>

    Let's avoid nesting by dealing with failure first:
    ```
    if (urls.empty()) {
       return Failure("...");
    }
    
    ...
    ```


- Jie Yu


On Aug. 23, 2018, 10:44 p.m., Liangyu Zhao wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68454/
> -----------------------------------------------------------
> 
> (Updated Aug. 23, 2018, 10:44 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, Gilbert Song, Jie Yu, Joseph
Wu, and Qian Zhang.
> 
> 
> Bugs: MESOS-9159
>     https://issues.apache.org/jira/browse/MESOS-9159
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> DockerFetcher now fetches both V2S1 and V2S2 manifests to save on
> disk when agent is running on Windows. Linux part of the code in
> agent is unchanged. In addition to fetching from DockerHub,
> DockerFetcher now supports fetching from foreign URLs provided in
> V2S2 Docker image manifest.
> 
> 
> Diffs
> -----
> 
>   3rdparty/stout/include/stout/path.hpp ef5a2f3d14cc8cf7416ded5cad00d30a49fd5cf7 
>   3rdparty/stout/tests/path_tests.cpp 452865b919c0d3644eb0ece0e17e402318aaff41 
>   src/uri/fetchers/docker.cpp 55ca118660872a933a2dc186723bec6a39ee80f7 
> 
> 
> Diff: https://reviews.apache.org/r/68454/diff/2/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Liangyu Zhao
> 
>


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