> On July 12, 2017, 1:47 a.m., Jiang Yan Xu wrote: > > src/slave/gc.hpp > > Lines 102 (patched) > > <https://reviews.apache.org/r/53479/diff/4/?file=1774290#file1774290line115> > > > > Indent less by two spaces.
Done. > On July 12, 2017, 1:47 a.m., Jiang Yan Xu wrote: > > src/slave/gc.hpp > > Line 120 (original), 111-115 (patched) > > <https://reviews.apache.org/r/53479/diff/4/?file=1774290#file1774290line124> > > > > 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; > > ``` > > > > ? A+ comment, storing ptr instead. > On July 12, 2017, 1:47 a.m., Jiang Yan Xu wrote: > > src/slave/gc.cpp > > Line 63 (original), 71 (patched) > > <https://reviews.apache.org/r/53479/diff/4/?file=1774291#file1774291line71> > > > > 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. Agree, fixed. > On July 12, 2017, 1:47 a.m., Jiang Yan Xu wrote: > > src/slave/gc.cpp > > Line 77 (original), 86 (patched) > > <https://reviews.apache.org/r/53479/diff/4/?file=1774291#file1774291line86> > > > > This can be > > > > ``` > > return info->promise.future(); > > ``` > > > > if you don't construct the promise outside PathInfo. Fixed. > On July 12, 2017, 1:47 a.m., Jiang Yan Xu wrote: > > src/slave/gc.cpp > > Lines 176-177 (patched) > > <https://reviews.apache.org/r/53479/diff/4/?file=1774291#file1774291line181> > > > > IMO this is very obvious from the `async` call and the content os the > > labmda and probably no need for commenting. Removed. > On July 12, 2017, 1:47 a.m., Jiang Yan Xu wrote: > > src/slave/gc.cpp > > Lines 179 (patched) > > <https://reviews.apache.org/r/53479/diff/4/?file=1774291#file1774291line184> > > > > Better to call `onAny` and then `CHECK` that it's always ready in the > > continuation so the intention is explicit. Checked this with a conditional...not sure when CHECK is appropriate. > On July 12, 2017, 1:47 a.m., Jiang Yan Xu wrote: > > src/slave/gc.cpp > > Lines 180-181 (patched) > > <https://reviews.apache.org/r/53479/diff/4/?file=1774291#file1774291line185> > > > > Kill these lines. Done. > On July 12, 2017, 1:47 a.m., Jiang Yan Xu wrote: > > src/slave/gc.cpp > > Lines 193 (patched) > > <https://reviews.apache.org/r/53479/diff/4/?file=1774291#file1774291line198> > > > > Put it above declaration? Done. > On July 12, 2017, 1:47 a.m., Jiang Yan Xu wrote: > > src/slave/gc.cpp > > Lines 194-195 (patched) > > <https://reviews.apache.org/r/53479/diff/4/?file=1774291#file1774291line199> > > > > 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. Indexed in by the timeouts entry from the path. - Jacob ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53479/#review180261 ----------------------------------------------------------- On July 13, 2017, 9: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, 9: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 > >
