----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53479/#review180510 -----------------------------------------------------------
src/slave/gc.hpp Lines 122-123 (patched) <https://reviews.apache.org/r/53479/#comment255728> No need for pointer? Just `process::Promise<Nothing> promise;` which is default initialized. src/slave/gc.cpp Line 93 (original), 103 (patched) <https://reviews.apache.org/r/53479/#comment255731> const & src/slave/gc.cpp Lines 150-152 (patched) <https://reviews.apache.org/r/53479/#comment255732> This comment doesn't seem necessary, we may have missed this initially but retrospectively resetting `removing` before starting async rmdir feels straighforward. src/slave/gc.cpp Lines 153 (patched) <https://reviews.apache.org/r/53479/#comment255730> const & src/slave/gc.cpp Lines 155 (patched) <https://reviews.apache.org/r/53479/#comment255734> Also add the action taken here, which is ``` VLOG(1) << "Skipping deletion of '" << info-> path << "' as it is already in progress"; ``` src/slave/gc.cpp Lines 159 (patched) <https://reviews.apache.org/r/53479/#comment255735> Not yours but quoting the path makes logging more consistent. Also move this into the lambda above each rmdir because this log does suggest the immediate rmdir for this path? src/slave/gc.cpp Lines 163 (patched) <https://reviews.apache.org/r/53479/#comment255733> I think the name and type `bool removing` is self-evident. src/slave/gc.cpp Lines 205 (patched) <https://reviews.apache.org/r/53479/#comment255737> `CHECK_READY(result);`? src/slave/gc.cpp Lines 206 (patched) <https://reviews.apache.org/r/53479/#comment255740> Kill this since we have the CHECK_READY. src/slave/gc.cpp Lines 207 (patched) <https://reviews.apache.org/r/53479/#comment255736> const & src/slave/gc.cpp Lines 210 (patched) <https://reviews.apache.org/r/53479/#comment255738> Use `CHECK_EQ(timeouts.erase(info->path), 1);`? - Jiang Yan Xu On July 13, 2017, 2:48 p.m., Jacob Janco wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/53479/ > ----------------------------------------------------------- > > (Updated July 13, 2017, 2:48 p.m.) > > > Review request for mesos, Benjamin Mahler and Jiang Yan Xu. > > > Bugs: MESOS-6549 > https://issues.apache.org/jira/browse/MESOS-6549 > > > Repository: mesos > > > Description > ------- > > - Previously, rmdir operations were serialized > through the garbage collector. Expensive removal > events had the potential to delay task launch. > - MESOS-6549 > > > Diffs > ----- > > src/slave/gc.hpp 5ea82456cffa374203371f227b6f0ee00296553e > src/slave/gc.cpp 961f547793984449ea113d9664b12b5033638c58 > > > Diff: https://reviews.apache.org/r/53479/diff/6/ > > > Testing > ------- > > `./bin/mesos-tests.sh --gtest_filter="GarbageCollector*" --gtest_repeat=100 > --gtest_break_on_failure` > > > Thanks, > > Jacob Janco > >
