----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/6704/#review10671 -----------------------------------------------------------
src/slave/flags.hpp <https://reviews.apache.org/r/6704/#comment22960> I think we should mention that this is actually just an upper bound, but as usage goes up, this number changes (i.e., explain the "age" function). src/slave/flags.hpp <https://reviews.apache.org/r/6704/#comment22948> No need for this newline. src/slave/flags.hpp <https://reviews.apache.org/r/6704/#comment22949> It will be a nice addition to be able to make all of these doubles either seconds, hours, or more likely, "duration" objects. Technical debt. :/ src/slave/gc.cpp <https://reviews.apache.org/r/6704/#comment22950> Include vector in this file. src/slave/gc.cpp <https://reviews.apache.org/r/6704/#comment22961> It would be more clear to factor the pair<string, Promise<bool>*> out into a struct. src/slave/gc.cpp <https://reviews.apache.org/r/6704/#comment22968> Why the 'creator' bit? src/slave/gc.cpp <https://reviews.apache.org/r/6704/#comment22966> I'll suggest 'reset' instead of 'next' for your deliberation. src/slave/gc.cpp <https://reviews.apache.org/r/6704/#comment22967> No need for ending period here! ;) Ask me later why. src/slave/gc.cpp <https://reviews.apache.org/r/6704/#comment22963> { src/slave/slave.hpp <https://reviews.apache.org/r/6704/#comment22958> Is usage a percent? src/slave/slave.cpp <https://reviews.apache.org/r/6704/#comment22954> s/checkDiskUsage/getDiskUsage/ src/slave/slave.cpp <https://reviews.apache.org/r/6704/#comment22956> Why not s/capacity/usage/? src/slave/slave.cpp <https://reviews.apache.org/r/6704/#comment22955> s/garbageCollect/checkDiskUsage/ src/slave/slave.cpp <https://reviews.apache.org/r/6704/#comment22957> It is a bit weird to see a Future<Try<>>, but I'm guessing you plan to eliminate that once you make the functions return a Future directly via the async stuff? src/tests/gc_tests.cpp <https://reviews.apache.org/r/6704/#comment22952> YES YES YES YES YES! I so much prefer this! We made createTask be so damn simple to reason about. Hurray! Note, eventually one could imagine making this a nice abstraction that all tests could use to creates tasks. - Benjamin Hindman On Aug. 21, 2012, 11:20 p.m., Vinod Kone wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/6704/ > ----------------------------------------------------------- > > (Updated Aug. 21, 2012, 11:20 p.m.) > > > Review request for mesos, Benjamin Hindman, John Sirois, and Ben Mahler. > > > Description > ------- > > Added the ability in the slave to GC executor directories based on the > current disk usage. > > Currently, it uses a very simple algorithm to decide which directories to > delete. > > > This addresses bug MESOS-254. > https://issues.apache.org/jira/browse/MESOS-254 > > > Diffs > ----- > > src/slave/constants.hpp ab83972 > src/slave/flags.hpp 0c7917f > src/slave/gc.hpp 6704742 > src/slave/gc.cpp 9c01024 > src/slave/slave.hpp 10c537b > src/slave/slave.cpp 4efd41e > src/tests/gc_tests.cpp 2f0bdde > third_party/libprocess/include/stout/os.hpp b1eceb3 > > Diff: https://reviews.apache.org/r/6704/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Vinod Kone > >
