----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53479/#review155639 -----------------------------------------------------------
Let's see if we can add some new tests. src/slave/gc.hpp (line 75) <https://reviews.apache.org/r/53479/#comment225657> Need to update the documentation here? We can make clear that if the the `path` is in the progress of being removed, the future is going to be satisfied with `false` when the removal finishes. The future is going to fail if the the removal is unsuccessful. src/slave/gc.hpp (line 120) <https://reviews.apache.org/r/53479/#comment225770> 'removal' and 'rm(dir)' sound too synonymous... To distinguish the two promises, perhaps name this one `gc` (to indicate it's for the overall gc and gc implies a sense of "scheduled for the future" to me). In any case we should use comments to make clear what is for what. e.g., ``` // The promise for the scheduled gc of this path. const process::Owned<process::Promise<Nothing>> gc; ``` src/slave/gc.hpp (lines 121 - 124) <https://reviews.apache.org/r/53479/#comment225658> This comment could be higher-level IMO. ``` This optional promise that tracks the progress of rmdir; it's none before rmdir starts. ``` src/slave/gc.hpp (line 125) <https://reviews.apache.org/r/53479/#comment225841> This rmdir is added to "account for the case when `unschedule` is called on a directory being removed" but it looks a bit strange to conflating the two: one may wonder "why is the progress of rmdir related to unschedules". To decouple the two, we can remove the bool: ``` Option<const process::Owned<process::Promise<Nothing>>> rmdir; ``` and define return value in `unschedule()`. I'll comment there. src/slave/gc.hpp (line 128) <https://reviews.apache.org/r/53479/#comment225836> Put it below `remove()`. src/slave/gc.hpp (lines 128 - 131) <https://reviews.apache.org/r/53479/#comment225837> This is too low-level for a summary IMO. Some of the details, if necessary, could be put in the method definition. src/slave/gc.cpp (lines 64 - 66) <https://reviews.apache.org/r/53479/#comment225772> 4-space indentation. src/slave/gc.cpp (line 80) <https://reviews.apache.org/r/53479/#comment225839> We probably shouldn't move the cancel here, see comments in `reset()`. src/slave/gc.cpp (line 102) <https://reviews.apache.org/r/53479/#comment225773> Need comments here. Something like ``` If rmdir is in progress, we can not completely "save" it from being removed; the best we can do is to wait until it's finished. ``` src/slave/gc.cpp (line 103) <https://reviews.apache.org/r/53479/#comment225842> Here we can do ``` // Return `false` to be consistent with the behavior when `unschedule` is called after the path is removed. return info.rmdir->future() .then([]() { return false; }); ``` src/slave/gc.cpp (lines 117 - 118) <https://reviews.apache.org/r/53479/#comment225840> Not yours but this should probaly be ``` UNREACHABLE() << "Inconsistent state across 'paths' and 'timeouts'"; ``` src/slave/gc.cpp (lines 125 - 130) <https://reviews.apache.org/r/53479/#comment225804> As it's written this way (along with the single path `_remove()`), in each batch of `rmdir()`s, the quickest one is going to trigger the next reset, by which time the head of the `paths` could be the same batch. We won't have this problem if we invoke `_remove()` once per "batch", in which case we don't need to change this method here. src/slave/gc.cpp (line 142) <https://reviews.apache.org/r/53479/#comment225797> Nit: as a contination, move it below `remove()`? src/slave/gc.cpp (line 168) <https://reviews.apache.org/r/53479/#comment225803> src/slave/gc.cpp (line 175) <https://reviews.apache.org/r/53479/#comment225775> src/slave/gc.cpp (lines 175 - 180) <https://reviews.apache.org/r/53479/#comment225791> I think we actually have to use the following? ``` map<process::Timeout, hashmap<std::string, PathInfo>> paths; ``` Because we rmdir asynchronously, when it's finished we need to retrieve the same `PathInfo` by both the removalTime and the path! If we do that, we can access all entries by reference so we won't have this problem. However we need to be cautious in cleaning up the outer entry when the inner map is all cleaned up. src/slave/gc.cpp (line 177) <https://reviews.apache.org/r/53479/#comment225774> No space. src/slave/gc.cpp (line 186) <https://reviews.apache.org/r/53479/#comment225793> Would this be an abnormal case? If so we can log a warning. src/slave/gc.cpp (lines 200 - 204) <https://reviews.apache.org/r/53479/#comment225795> 4 space indentation and put `self()` on the next line. Also, it looks like instead of having one callback for each rmdir, we should be probably `await` on all of them and have the callback for the overall result. This is because we need to reset the timer only when the batch of `rmdir`s completes. Of course then the overall callback needs to be informed about which paths this list of futures are for. i.e., something like this ``` hashmap<string, Future<Nothing>> futures; ... In foreach: futures[info.path] = async(&os::rmdir, info.path, true, true, true); ... await(values.values()) .onAny(...); ``` The `_remove()` method can be defined as: ``` void GarbageCollectorProcess::_remove( const Timeout& removalTime, const hashmap<string, Future<Nothing>>& futures) { } ``` src/slave/gc.cpp (line 203) <https://reviews.apache.org/r/53479/#comment225802> It's probably safe to pass a copy of `PathInfo` here but the fact that it's safe it's not obvious. We can probably just pass a `path` so in the continuation we can use `removalTime` and `path` to get the `PathInfo` back and we can just `CHECK` to assert that the entry should be there and add a comment on why it should be there (because we don't erase the any entry with a pending `rmdir`, we wait until it's finished). src/slave/gc.cpp (lines 212 - 213) <https://reviews.apache.org/r/53479/#comment225796> Because you ``` if (it->second.rmdir.isSome()) { continue; } ``` above, it's possible for method to not trigger a `reset()` at all if we move it like this? - Jiang Yan Xu On Nov. 4, 2016, 10:25 a.m., Jacob Janco wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/53479/ > ----------------------------------------------------------- > > (Updated Nov. 4, 2016, 10:25 a.m.) > > > Review request for mesos and Jiang Yan Xu. > > > Bugs: MESOS-6549 > https://issues.apache.org/jira/browse/MESOS-6549 > > > Repository: mesos > > > Description > ------- > > Perform GC asynchronously. > > > Diffs > ----- > > src/slave/gc.hpp 5ea82456cffa374203371f227b6f0ee00296553e > src/slave/gc.cpp 961f547793984449ea113d9664b12b5033638c58 > > Diff: https://reviews.apache.org/r/53479/diff/ > > > Testing > ------- > > `make check` > > > Thanks, > > Jacob Janco > >
