----------------------------------------------------------- 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 > >