> On Oct. 26, 2018, 11:20 p.m., Joseph Wu wrote: > > src/slave/containerizer/fetcher.cpp > > Lines 1030-1031 (patched) > > <https://reviews.apache.org/r/69171/diff/1/?file=2102490#file2102490line1030> > > > > 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()) { > > ... > > ```
Fixed. - Andrei ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69171/#review210115 ----------------------------------------------------------- On Oct. 29, 2018, 10:33 p.m., Andrei Budnik wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69171/ > ----------------------------------------------------------- > > (Updated Oct. 29, 2018, 10:33 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/2/ > > > Testing > ------- > > 1. sudo make check (Fedora 25) > 2. internal CI > > > Thanks, > > Andrei Budnik > >
