> On July 14, 2017, 8:47 a.m., Jiang Yan Xu wrote: > > src/slave/gc.hpp > > Lines 122-123 (patched) > > <https://reviews.apache.org/r/53479/diff/6/?file=1776250#file1776250line123> > > > > No need for pointer? > > > > Just `process::Promise<Nothing> promise;` which is default initialized.
My mistake, I added an overload to Promise to help with the PathInfo check since we're not checking the pointer anymore. > On July 14, 2017, 8:47 a.m., Jiang Yan Xu wrote: > > src/slave/gc.cpp > > Lines 150-152 (patched) > > <https://reviews.apache.org/r/53479/diff/6/?file=1776251#file1776251line156> > > > > This comment doesn't seem necessary, we may have missed this initially > > but retrospectively resetting `removing` before starting async rmdir feels > > straighforward. Cool, removing. > On July 14, 2017, 8:47 a.m., Jiang Yan Xu wrote: > > src/slave/gc.cpp > > Lines 155 (patched) > > <https://reviews.apache.org/r/53479/diff/6/?file=1776251#file1776251line161> > > > > Also add the action taken here, which is > > > > ``` > > VLOG(1) << "Skipping deletion of '" << info-> path << "' as it is > > already in progress"; > > ``` Fixed. > On July 14, 2017, 8:47 a.m., Jiang Yan Xu wrote: > > src/slave/gc.cpp > > Lines 159 (patched) > > <https://reviews.apache.org/r/53479/diff/6/?file=1776251#file1776251line165> > > > > 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? That makes more sense, moving the log line there. > On July 14, 2017, 8:47 a.m., Jiang Yan Xu wrote: > > src/slave/gc.cpp > > Lines 163 (patched) > > <https://reviews.apache.org/r/53479/diff/6/?file=1776251#file1776251line169> > > > > I think the name and type `bool removing` is self-evident. Removed comment. > On July 14, 2017, 8:47 a.m., Jiang Yan Xu wrote: > > src/slave/gc.cpp > > Lines 205 (patched) > > <https://reviews.apache.org/r/53479/diff/6/?file=1776251#file1776251line213> > > > > `CHECK_READY(result);`? Added. > On July 14, 2017, 8:47 a.m., Jiang Yan Xu wrote: > > src/slave/gc.cpp > > Lines 206 (patched) > > <https://reviews.apache.org/r/53479/diff/6/?file=1776251#file1776251line214> > > > > Kill this since we have the CHECK_READY. Removed. > On July 14, 2017, 8:47 a.m., Jiang Yan Xu wrote: > > src/slave/gc.cpp > > Lines 210 (patched) > > <https://reviews.apache.org/r/53479/diff/6/?file=1776251#file1776251line218> > > > > Use `CHECK_EQ(timeouts.erase(info->path), 1);`? Also fixed this in `unschedule` - Jacob ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53479/#review180510 ----------------------------------------------------------- On July 15, 2017, 1:20 a.m., Jacob Janco wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/53479/ > ----------------------------------------------------------- > > (Updated July 15, 2017, 1:20 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/8/ > > > Testing > ------- > > `./bin/mesos-tests.sh --gtest_filter="GarbageCollector*" --gtest_repeat=100 > --gtest_break_on_failure` > > > Thanks, > > Jacob Janco > >
