----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5072/#review8178 -----------------------------------------------------------
src/common/time.hpp <https://reviews.apache.org/r/5072/#comment17679> s/3600*/3600 */ src/slave/slave.hpp <https://reviews.apache.org/r/5072/#comment17680> const & src/slave/slave.cpp <https://reviews.apache.org/r/5072/#comment17681> What is this needed for? src/slave/slave.cpp <https://reviews.apache.org/r/5072/#comment17682> s/logs/directories/ src/slave/slave.cpp <https://reviews.apache.org/r/5072/#comment17689> My two cents: I'm thinking here that garbageCollectSlaveDirs is a function that's going to get called multiple times, that's why it's abstracted into a function and that's why it's named the way that it is. In fact, what's really happening here is this: onStartupLookForAllDirectoriesUnderOurWorkDirectoryAndScheduleThemForGarbageCollectionAfterATimeout. Honestly, I'd rather just see that as a comment and the code right here then have that abstracted away someplace else. What can be abstracted away is a call to schedule a directory to get garbaged collected (that this code will use). My four cents: I would have put garbage collection of directories into their own process called GarbageCollector with a single function GarbageCollector::schedule that takes a function that schedules a directory for garbage collection. ;) src/slave/slave.cpp <https://reviews.apache.org/r/5072/#comment17683> Old branch? src/slave/slave.cpp <https://reviews.apache.org/r/5072/#comment17684> Ditto as last comment. src/slave/slave.cpp <https://reviews.apache.org/r/5072/#comment17688> Why do you need the copy here? src/slave/slave.cpp <https://reviews.apache.org/r/5072/#comment17685> You need to rebase this on trunk please. src/slave/slave.cpp <https://reviews.apache.org/r/5072/#comment17686> hours timeout(conf.get<double>(...)); src/slave/slave.cpp <https://reviews.apache.org/r/5072/#comment17687> hours timeout(conf.get<double>(...)); src/tests/slave_tests.cpp <https://reviews.apache.org/r/5072/#comment17690> Put these guys before the includes that use "". src/tests/slave_tests.cpp <https://reviews.apache.org/r/5072/#comment17691> Why '/var/tmp'? What's the problem with the default? src/tests/slave_tests.cpp <https://reviews.apache.org/r/5072/#comment17692> s/conf/configurator/ src/tests/slave_tests.cpp <https://reviews.apache.org/r/5072/#comment17693> Can all this be setup once in the constructor versus for each test? src/tests/slave_tests.cpp <https://reviews.apache.org/r/5072/#comment17694> Ugh, this is a nasty side effect to think about ... it would be better to do the WAIT_UNTIL outside of te launchTask call! src/tests/slave_tests.cpp <https://reviews.apache.org/r/5072/#comment17695> Seems like you can just get away with a Configuration object rather than a Configurator. src/tests/slave_tests.cpp <https://reviews.apache.org/r/5072/#comment17696> A comment as to why you're starting the slave after the driver might be illuminating. src/tests/slave_tests.cpp <https://reviews.apache.org/r/5072/#comment17697> hours timeout(slave::GC_TIMEOUT_HOURS); src/tests/slave_tests.cpp <https://reviews.apache.org/r/5072/#comment17698> In this case, the launchTask will not wait for resourceOffersCall because it has already occurred, hence the reason to not abstract that away. If you want to get multiple resourceOfferCalls, consider using a volatile int that you increment each time a resourceOffer call occurs. src/tests/slave_tests.cpp <https://reviews.apache.org/r/5072/#comment17699> Kill this if it's not used please. src/tests/slave_tests.cpp <https://reviews.apache.org/r/5072/#comment17700> hours timeout(...); - Benjamin On 2012-05-14 18:09:28, Vinod Kone wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/5072/ > ----------------------------------------------------------- > > (Updated 2012-05-14 18:09:28) > > > Review request for mesos, Benjamin Hindman, John Sirois, and Brian Wickman. > > > Summary > ------- > > This is the first cut for GC inside the slave. > > There are 2 kinds of gc going on > > --> Executor work dirs -- These get deleted whenever (after a timeout) an > executor exits/shutdown > --> Old slave dirs -- These get deleted when the slave gets registered for > the first time on a startup. > > > Diffs > ----- > > src/Makefile.am cd503a8 > src/common/seconds.hpp 5ae088a > src/common/time.hpp PRE-CREATION > src/common/timer.hpp 71dc493 > src/common/utils.hpp 1d81e21 > src/log/coordinator.hpp b881c6a > src/log/log.hpp 79bb738 > src/log/network.hpp 6ce6a53 > src/master/frameworks_manager.hpp 31d708b > src/slave/constants.hpp f0c8679 > src/slave/slave.hpp 08a29d8 > src/slave/slave.cpp 09a8396 > src/tests/base_zookeeper_test.hpp f35acc0 > src/tests/fault_tolerance_tests.cpp 6772daf > src/tests/slave_tests.cpp PRE-CREATION > src/zookeeper/group.hpp 695a8ab > src/zookeeper/zookeeper.hpp 5043a64 > > Diff: https://reviews.apache.org/r/5072/diff > > > Testing > ------- > > make check scucceeds. > > Yet to write GC specific tests. > > > Thanks, > > Vinod > >
