----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53479/#review180261 -----------------------------------------------------------
Looking good! Just some minor issues. src/slave/gc.hpp Line 108 (original), 97 (patched) <https://reviews.apache.org/r/53479/#comment255344> Why does this become public? src/slave/gc.hpp Line 111 (original), 100 (patched) <https://reviews.apache.org/r/53479/#comment255342> Can this be const? src/slave/gc.hpp Line 112 (original), 101 (patched) <https://reviews.apache.org/r/53479/#comment255343> Do we need to pass this in? src/slave/gc.hpp Lines 102 (patched) <https://reviews.apache.org/r/53479/#comment255345> Indent less by two spaces. src/slave/gc.hpp Line 120 (original), 111-115 (patched) <https://reviews.apache.org/r/53479/#comment255347> Storing a `Owned<bool> removing` looks odd, instead we we can just have ``` struct PathInfo { ... process::Promise<Nothing> promise; bool removing; } ``` but store `Owned<PathInfo>` in ``` Multimap<process::Timeout, process::Owned<PathInfo>> paths; ``` ? src/slave/gc.cpp Line 63 (original), 71 (patched) <https://reviews.apache.org/r/53479/#comment255348> We don't need to do these if we just store `Owned<PathInfo>`. In fact, it probably doesn't even need a custom constructor. It can be default constructed. src/slave/gc.cpp Line 77 (original), 86 (patched) <https://reviews.apache.org/r/53479/#comment255349> This can be ``` return info->promise.future(); ``` if you don't construct the promise outside PathInfo. src/slave/gc.cpp Lines 176-177 (patched) <https://reviews.apache.org/r/53479/#comment255361> IMO this is very obvious from the `async` call and the content os the labmda and probably no need for commenting. src/slave/gc.cpp Lines 179 (patched) <https://reviews.apache.org/r/53479/#comment255360> Better to call `onAny` and then `CHECK` that it's always ready in the continuation so the intention is explicit. src/slave/gc.cpp Lines 180-181 (patched) <https://reviews.apache.org/r/53479/#comment255359> Kill these lines. src/slave/gc.cpp Lines 193 (patched) <https://reviews.apache.org/r/53479/#comment255362> Put it above declaration? src/slave/gc.cpp Lines 194-195 (patched) <https://reviews.apache.org/r/53479/#comment255363> No need for passing both. In fact, `infos` is not used here when we should: there could be other entries of the same timeout added while the removal is ongoing but `infos` is the source of truth on what you just removed. - Jiang Yan Xu On July 11, 2017, 11:28 a.m., Jacob Janco wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/53479/ > ----------------------------------------------------------- > > (Updated July 11, 2017, 11:28 a.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/4/ > > > Testing > ------- > > `./bin/mesos-tests.sh --gtest_filter="GarbageCollector*" --gtest_repeat=100 > --gtest_break_on_failure` > > > Thanks, > > Jacob Janco > >
