----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53479/#review178839 -----------------------------------------------------------
src/slave/gc.hpp Lines 126-131 (patched) <https://reviews.apache.org/r/53479/#comment253124> There is still a bit of unclarity between the two, probably come the ambiguity of the concepts gc vs. scheduled (pending, not in progress), rmdir (in progress), etc. Also the fact that when a path is removed we have to set two promises is a bit odd. I commented on a possible simplication below and with that I think we can just leave the field `promise` as is and add a `bool removing`: ``` const process::Owned<process::Promise<Nothing>> promise; bool removing = false; ``` Let's discuss it. src/slave/gc.cpp Line 63 (original), 71 (patched) <https://reviews.apache.org/r/53479/#comment253117> This is the promise that sets `gc` in the changed `PathInfo` (as opposed to `removal` there) right? If the callers should not be concerns about the distinction, perhaps just keep the name `promise`. Anyhow, this comment might not be relevant if we use a `bool removing`. src/slave/gc.cpp Lines 106-107 (patched) <https://reviews.apache.org/r/53479/#comment253118> The statement sounds like describing a race condition but rather returning `false` is just because the gc is not prevented? Perhaps just consolidate with the comment about that we "return false (gc is not unscheduled) after rmdir completes?" src/slave/gc.cpp Lines 108 (patched) <https://reviews.apache.org/r/53479/#comment253125> If we follow the suggestions below, here it could be ``` if (info.removing) { return info.promise->future() .then([]() { return false; }); } ``` ? src/slave/gc.cpp Line 153 (original), 149-178 (patched) <https://reviews.apache.org/r/53479/#comment253129> Would this be simpler? ``` list<PathInfo> infos = paths.get(removalTime); auto rmdirs = [infos]() { foreach (const PathInfo& info, infos) { LOG(INFO) << "Deleting " << info.path; // ... Try<Nothing> rmdir = os::rmdir(info.path, true, true, true); if (rmdir.isError()) { LOG(WARNING) << "Failed to delete '" << info.path << "': " << rmdir.error(); info.promise->fail(rmdir.error()); } else { LOG(INFO) << "Deleted '" << info.path << "'"; info.promise->set(rmdir.get()); } } } async(rmdirs); .then(defer(self(),&Self::_remove, infos); ``` Since our main objective is to move rmdir out of the `GarbageCollectorProcess` so perhaps one async call to sequentially remove all of the paths as it was done before when it can simply the code is worth it. In practice there likely won't be many paths sharing the same scheduled gc time anyway. src/slave/gc.cpp Lines 212 (patched) <https://reviews.apache.org/r/53479/#comment253128> Blank line above. - Jiang Yan Xu On May 2, 2017, 2:33 p.m., Jacob Janco wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/53479/ > ----------------------------------------------------------- > > (Updated May 2, 2017, 2:33 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/3/ > > > Testing > ------- > > `./bin/mesos-tests.sh --gtest_filter="GarbageCollector*" --gtest_repeat=100 > --gtest_break_on_failure` > > > Thanks, > > Jacob Janco > >
