----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69171/#review210115 -----------------------------------------------------------
src/slave/containerizer/fetcher.cpp Lines 1030-1031 (patched) <https://reviews.apache.org/r/69171/#comment294752> Hum... I think this will work as intended, but the logic and comment are slight off. A cache should be considered downloaded if `completion().isReady()`. In case the download fails, `!completion.isPending()` will be true. However every invocation of `entry.get()->fail()` is synchronized inside the `FetcherProcess`. And every time `fail()` is called, the line is followed by a `cache.remove(...)`, so it should be impossible to find a Failed completion here. Perhaps this will be slight more consistent: ``` // The FetcherProcess will always remove a failed download // synchronously after marking this future as failed. CHECK(!entry.get().completion().isFailed()); // Validate the cache file, if it has been downloaded. if (entry.get().completion().isReady()) { ... ``` src/slave/containerizer/fetcher.cpp Lines 1070-1074 (original), 1083-1087 (patched) <https://reviews.apache.org/r/69171/#comment294751> The new case will have a similar reason to (3). Maybe something like: ``` In (4) we explicitly only validate a cache file if the future is ready (i.e the file has been downloaded). ``` - Joseph Wu On Oct. 25, 2018, 1:47 p.m., Andrei Budnik wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69171/ > ----------------------------------------------------------- > > (Updated Oct. 25, 2018, 1:47 p.m.) > > > Review request for mesos, Gilbert Song, James Peach, and Joseph Wu. > > > Bugs: MESOS-7474 > https://issues.apache.org/jira/browse/MESOS-7474 > > > Repository: mesos > > > Description > ------- > > Previously, missing cache files could lead to task launch failures. > This patch introduces validation of cache files which happens each > time a downloaded cache entry is requested via the `get()` method. > If validation fails, `get()` returns `None()`, so that the fetcher > retries downloading URI. Currently, we only check the existence of > the cache file during validation. > > > Diffs > ----- > > src/slave/containerizer/fetcher.cpp > e848c86261b75e5549e80276541e5932162fc012 > src/slave/containerizer/fetcher_process.hpp > 56ed6e5455b7d23b4ce84873fe6734b4213df691 > > > Diff: https://reviews.apache.org/r/69171/diff/1/ > > > Testing > ------- > > 1. sudo make check (Fedora 25) > 2. internal CI > > > Thanks, > > Andrei Budnik > >
