> On 2012-05-29 22:49:04, Benjamin Hindman wrote: > > src/common/time.hpp, line 89 > > <https://reviews.apache.org/r/5072/diff/3/?file=108826#file108826line89> > > > > s/3600*/3600 */
hawk! fixed > On 2012-05-29 22:49:04, Benjamin Hindman wrote: > > src/slave/slave.hpp, line 149 > > <https://reviews.apache.org/r/5072/diff/3/?file=108834#file108834line149> > > > > const & done > On 2012-05-29 22:49:04, Benjamin Hindman wrote: > > src/slave/slave.cpp, line 29 > > <https://reviews.apache.org/r/5072/diff/3/?file=108835#file108835line29> > > > > What is this needed for? good catch. no longer needed now that modtime() is moved to utils. > On 2012-05-29 22:49:04, Benjamin Hindman wrote: > > src/slave/slave.cpp, line 203 > > <https://reviews.apache.org/r/5072/diff/3/?file=108835#file108835line203> > > > > s/logs/directories/ done > On 2012-05-29 22:49:04, Benjamin Hindman wrote: > > src/slave/slave.cpp, line 1414 > > <https://reviews.apache.org/r/5072/diff/3/?file=108835#file108835line1414> > > > > Old branch? yea. likely > On 2012-05-29 22:49:04, Benjamin Hindman wrote: > > src/slave/slave.cpp, line 1427 > > <https://reviews.apache.org/r/5072/diff/3/?file=108835#file108835line1427> > > > > Ditto as last comment. yes > On 2012-05-29 22:49:04, Benjamin Hindman wrote: > > src/slave/slave.cpp, line 1448 > > <https://reviews.apache.org/r/5072/diff/3/?file=108835#file108835line1448> > > > > Why do you need the copy here? because framework->destroyExecutor(), below, deletes executor > On 2012-05-29 22:49:04, Benjamin Hindman wrote: > > src/slave/slave.cpp, line 1489 > > <https://reviews.apache.org/r/5072/diff/3/?file=108835#file108835line1489> > > > > You need to rebase this on trunk please. > On 2012-05-29 22:49:04, Benjamin Hindman wrote: > > src/slave/slave.cpp, line 1511 > > <https://reviews.apache.org/r/5072/diff/3/?file=108835#file108835line1511> > > > > hours timeout(conf.get<double>(...)); good point. done > On 2012-05-29 22:49:04, Benjamin Hindman wrote: > > src/slave/slave.cpp, line 1523 > > <https://reviews.apache.org/r/5072/diff/3/?file=108835#file108835line1523> > > > > hours timeout(conf.get<double>(...)); done > On 2012-05-29 22:49:04, Benjamin Hindman wrote: > > src/tests/slave_tests.cpp, lines 33-34 > > <https://reviews.apache.org/r/5072/diff/3/?file=108838#file108838line33> > > > > Put these guys before the includes that use "". done > On 2012-05-29 22:49:04, Benjamin Hindman wrote: > > src/tests/slave_tests.cpp, line 241 > > <https://reviews.apache.org/r/5072/diff/3/?file=108838#file108838line241> > > > > hours timeout(slave::GC_TIMEOUT_HOURS); done > On 2012-05-29 22:49:04, Benjamin Hindman wrote: > > src/tests/slave_tests.cpp, line 217 > > <https://reviews.apache.org/r/5072/diff/3/?file=108838#file108838line217> > > > > A comment as to why you're starting the slave after the driver might be > > illuminating. no particular reason. does it matter? > On 2012-05-29 22:49:04, Benjamin Hindman wrote: > > src/tests/slave_tests.cpp, line 275 > > <https://reviews.apache.org/r/5072/diff/3/?file=108838#file108838line275> > > > > Kill this if it's not used please. killed > On 2012-05-29 22:49:04, Benjamin Hindman wrote: > > src/tests/slave_tests.cpp, line 325 > > <https://reviews.apache.org/r/5072/diff/3/?file=108838#file108838line325> > > > > hours timeout(...); done > On 2012-05-29 22:49:04, Benjamin Hindman wrote: > > src/tests/slave_tests.cpp, line 82 > > <https://reviews.apache.org/r/5072/diff/3/?file=108838#file108838line82> > > > > Why '/var/tmp'? What's the problem with the default? this hoop is to get access to the work directory. it has nothing to do with the directory name. makes sense? > On 2012-05-29 22:49:04, Benjamin Hindman wrote: > > src/tests/slave_tests.cpp, line 146 > > <https://reviews.apache.org/r/5072/diff/3/?file=108838#file108838line146> > > > > Ugh, this is a nasty side effect to think about ... it would be better > > to do the WAIT_UNTIL outside of te launchTask call! ok. moved > On 2012-05-29 22:49:04, Benjamin Hindman wrote: > > src/tests/slave_tests.cpp, line 184 > > <https://reviews.apache.org/r/5072/diff/3/?file=108838#file108838line184> > > > > Seems like you can just get away with a Configuration object rather > > than a Configurator. done > On 2012-05-29 22:49:04, Benjamin Hindman wrote: > > src/tests/slave_tests.cpp, line 254 > > <https://reviews.apache.org/r/5072/diff/3/?file=108838#file108838line254> > > > > 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. umm, the line above resets the resourceOffersCall trigger, to force the wait. am i missing something? > On 2012-05-29 22:49:04, Benjamin Hindman wrote: > > src/tests/slave_tests.cpp, line 84 > > <https://reviews.apache.org/r/5072/diff/3/?file=108838#file108838line84> > > > > s/conf/configurator/ done > On 2012-05-29 22:49:04, Benjamin Hindman wrote: > > src/tests/slave_tests.cpp, line 85 > > <https://reviews.apache.org/r/5072/diff/3/?file=108838#file108838line85> > > > > Can all this be setup once in the constructor versus for each test? IIUC, constructor is also called per test!? did u mean SetupTestCase()? > On 2012-05-29 22:49:04, Benjamin Hindman wrote: > > src/slave/slave.cpp, line 404 > > <https://reviews.apache.org/r/5072/diff/3/?file=108835#file108835line404> > > > > 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. ;) I abstracted it away so that all GC related functions could be modularized (incase we want to change them in future since this is a short term fix). I also felt, it looked better to not stuff this inside registered(). but, if you feel strongly about it, i can fix it. i also like the idea of having GC as its own lib process, but we can spend time on it after slave-restart, because i suspect the gc logic/implementation is going to change. - Vinod ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5072/#review8178 ----------------------------------------------------------- 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 > >
