----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/6704/#review11304 -----------------------------------------------------------
src/slave/flags.hpp <https://reviews.apache.org/r/6704/#comment24317> As I mentioned before, I really would prefer that we mention that this number is really just an upper bound and changes based on disk usage, something like: Maximum amount of time to wait before cleaning up\n executor directories (e.g., 3days, 2 weeks, etc).\n Note that this delay may be shorter depending on\n available disk usage src/slave/gc.cpp <https://reviews.apache.org/r/6704/#comment24316> Previously I've embedded a struct like this directly into the class (just above the map that holds these). See src/zookeeper/group.cpp. src/slave/gc.cpp <https://reviews.apache.org/r/6704/#comment24314> The timer.creator() seems weird to me. What about capturing exactly the semantics you care about doing something like: if (timer.timeout().remaining() == Seconds(0) || removalTime < timer.timeout()) { src/slave/gc.cpp <https://reviews.apache.org/r/6704/#comment24312> const & src/slave/gc.cpp <https://reviews.apache.org/r/6704/#comment24313> > 0 src/slave/gc.cpp <https://reviews.apache.org/r/6704/#comment24311> s/.// src/slave/gc.cpp <https://reviews.apache.org/r/6704/#comment24310> const & src/slave/gc.cpp <https://reviews.apache.org/r/6704/#comment24315> What is the reason to dispatch here? src/slave/slave.hpp <https://reviews.apache.org/r/6704/#comment24309> I do not think we should proliferate this kind of thing. Please see the comment in the test itself as well. src/slave/slave.cpp <https://reviews.apache.org/r/6704/#comment24298> Please don't crash the slave if for whatever reason a file gets added to the directory that is not a number. src/slave/slave.cpp <https://reviews.apache.org/r/6704/#comment24308> We haven't come to a conclusion on the style for putting things inside of a CHECK (except for in tests). For now, please pull it out (like the deleted code) until we decide what we want to do. Thanks! src/slave/slave.cpp <https://reviews.apache.org/r/6704/#comment24318> s/atleast/at least/ src/tests/gc_tests.cpp <https://reviews.apache.org/r/6704/#comment24307> Once _checkDiskUsage becomes a lambda, you won't be able to test it this way ... any thoughts on how to write a test then? third_party/libprocess/include/stout/os.hpp <https://reviews.apache.org/r/6704/#comment24303> Rather than the weird wrapping here, why not put all of open(... on the newline? For example: Try<int> fd = open(path, O_RDWR | O_CREAT, S_IRUSR | S_IWUSR | S_IRGRP | S_IRWXO); third_party/libprocess/include/stout/os.hpp <https://reviews.apache.org/r/6704/#comment24302> Any reason not to call this du? - Benjamin Hindman On Sept. 10, 2012, 6:39 a.m., Vinod Kone wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/6704/ > ----------------------------------------------------------- > > (Updated Sept. 10, 2012, 6:39 a.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 380a2b9 > src/slave/flags.hpp e99ee8b > src/slave/gc.hpp 3760d09 > src/slave/gc.cpp 5212a41 > src/slave/slave.hpp b7ab2ab > src/slave/slave.cpp 3e2a8d5 > src/tests/gc_tests.cpp bb7416c > third_party/libprocess/include/process/timeout.hpp ce0dea7 > third_party/libprocess/include/stout/duration.hpp e303d2c > third_party/libprocess/include/stout/os.hpp df0f7ff > > Diff: https://reviews.apache.org/r/6704/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Vinod Kone > >
