You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Liangyu Zhao via Review Board <no...@reviews.apache.org> on 2018/08/21 21:29:52 UTC
Review Request 68453: Windows: Fetch version 2 scheme 2 Docker image
manifest.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68453/
-----------------------------------------------------------
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.
Diffs
-----
src/uri/fetchers/docker.cpp 55ca118660872a933a2dc186723bec6a39ee80f7
Diff: https://reviews.apache.org/r/68453/diff/1/
Testing
-------
Thanks,
Liangyu Zhao
Re: Review Request 68453: Windows: Fetch version 2 scheme 2 Docker
image manifest.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68453/#review207804
-----------------------------------------------------------
src/uri/fetchers/docker.cpp
Line 745 (original), 853-864 (patched)
<https://reviews.apache.org/r/68453/#comment291317>
This is probably more clear if we just inline `__fetchBlob` here:
```
if (code == http::Status::OK) {
return Nothing();
}
#ifdef __WINDOWS__
return urlFetchBlob(uri, directory, blobUri, authHeaders, code);
#else
return Failure(
"Unexpected HTTP response '" + http::Status::string(code) + "' "
"when trying to download the blob");
#endif
});
```
src/uri/fetchers/docker.cpp
Lines 774-776 (original), 893-908 (patched)
<https://reviews.apache.org/r/68453/#comment291316>
I think this is probably more clear like the following:
```
.then(defer(self(), [=](int code) {
if (code == http::Status::OK) {
return Nothing();
}
#ifdef __WINDOWS__
return urlFetchBlob(uri, directory, blobUri, authHeaders, code);
#else
return Failure(
"Unexpected HTTP response '" + http::Status::string(code) + "' "
"when trying to download the blob");
#endif
});
```
- Jie Yu
On Aug. 21, 2018, 9:29 p.m., Liangyu Zhao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68453/
> -----------------------------------------------------------
>
> (Updated Aug. 21, 2018, 9:29 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.
>
>
> Diffs
> -----
>
> src/uri/fetchers/docker.cpp 55ca118660872a933a2dc186723bec6a39ee80f7
>
>
> Diff: https://reviews.apache.org/r/68453/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Liangyu Zhao
>
>
Re: Review Request 68453: Windows: Fetch version 2 scheme 2 Docker
image manifest.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68453/#review207801
-----------------------------------------------------------
src/uri/fetchers/docker.cpp
Lines 432 (patched)
<https://reviews.apache.org/r/68453/#comment291299>
I would get rid of this default value. Let's just be explicit in the caller.
src/uri/fetchers/docker.cpp
Lines 647-681 (original), 665-700 (patched)
<https://reviews.apache.org/r/68453/#comment291305>
I suggest you move the logic to `saveSchema1Manfiest` helper, which will parse http response, check http code and content type, save file to directory, and return the parsed schema1 manifest.
This will be consistent with `saveSchema2Manfiest`
src/uri/fetchers/docker.cpp
Line 677 (original), 695-696 (patched)
<https://reviews.apache.org/r/68453/#comment291296>
nits: please do the following so that it's consistent with others in this file:
```
return Failure(
"Unsupported schema 1 manifest MIME type: " +
contentType.get());
```
src/uri/fetchers/docker.cpp
Line 683 (original), 702-703 (patched)
<https://reviews.apache.org/r/68453/#comment291297>
Ditto.
src/uri/fetchers/docker.cpp
Lines 727-752 (patched)
<https://reviews.apache.org/r/68453/#comment291304>
This basically hides errors. VLOG won't be visible to caller. I'd suggest the following:
```
return curl(...)
.then(defer(self(),
&Self::saveSchema2Manifest,
lambda::_1))
.then(defer(self(),
&Self::___fetch,
uri,
directory,
authHeaders,
lambda::_1));
```
`saveSchema2Manifest` will parse the http response, validate content type and return code, and write to the disk. It'll return parsed schema2 manifest, which will be fed into `___fetch`.
src/uri/fetchers/docker.cpp
Lines 791-795 (patched)
<https://reviews.apache.org/r/68453/#comment291301>
this can simply be `return os::write(...)`?
src/uri/fetchers/docker.cpp
Lines 799 (patched)
<https://reviews.apache.org/r/68453/#comment291306>
suggest rename this to `___fetch` as this is really a continuation of `__fetch`.
src/uri/fetchers/docker.cpp
Lines 803 (patched)
<https://reviews.apache.org/r/68453/#comment291307>
Why `Try` is needed here?
src/uri/fetchers/docker.cpp
Lines 855 (patched)
<https://reviews.apache.org/r/68453/#comment291309>
I'd use `.repair` instead since you only care about repair the Failure scenario.
src/uri/fetchers/docker.cpp
Lines 856 (patched)
<https://reviews.apache.org/r/68453/#comment291310>
I didn't find the implementation in this patch? We typically make sure each patch is self contained, but I understand this is a long chain and might not be easy to tweak. Feel free to ignore this
src/uri/fetchers/docker.cpp
Lines 897 (patched)
<https://reviews.apache.org/r/68453/#comment291308>
I'd use `.repair` instead
- Jie Yu
On Aug. 21, 2018, 9:29 p.m., Liangyu Zhao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68453/
> -----------------------------------------------------------
>
> (Updated Aug. 21, 2018, 9:29 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.
>
>
> Diffs
> -----
>
> src/uri/fetchers/docker.cpp 55ca118660872a933a2dc186723bec6a39ee80f7
>
>
> Diff: https://reviews.apache.org/r/68453/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Liangyu Zhao
>
>