You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Vinod Kone <vi...@gmail.com> on 2012/05/14 20:09:28 UTC
Re: Review Request: Slave Garbage Collection
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5072/
-----------------------------------------------------------
(Updated 2012-05-14 18:09:28.284385)
Review request for mesos, Benjamin Hindman, John Sirois, and Brian Wickman.
Changes
-------
added tests
Summary (updated)
-------
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 (updated)
-----
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
Re: Review Request: Slave Garbage Collection
Posted by Vinod Kone <vi...@gmail.com>.
> 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
>
>
Re: Review Request: Slave Garbage Collection
Posted by Benjamin Hindman <be...@berkeley.edu>.
> 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?
>
> Vinod Kone wrote:
> because framework->destroyExecutor(), below, deletes executor
The string will get copied where/when it needs to get copied automagically. In this case, it will happen when you do 'result.push_back(dir)' inside garbageCollectExecutorDir.
> 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?
>
> Vinod Kone wrote:
> this hoop is to get access to the work directory. it has nothing to do with the directory name. makes sense?
No, it doesn't. :( Why aren't you just using '/tmp/mesos'?
> 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?
>
> Vinod Kone wrote:
> IIUC, constructor is also called per test!? did u mean SetupTestCase()?
Yes, that is what I meant.
> 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.
>
> Vinod Kone wrote:
> no particular reason. does it matter?
No, but all of our other tests do it differently, so it stands out. Thus, if it doesn't matter, I'd probably start the slave before the driver.
> 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.
>
> Vinod Kone wrote:
> umm, the line above resets the resourceOffersCall trigger, to force the wait. am i missing something?
Oh woah yeah, totally missed that!
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5072/#review8178
-----------------------------------------------------------
On 2012-05-30 01:33:27, Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5072/
> -----------------------------------------------------------
>
> (Updated 2012-05-30 01:33:27)
>
>
> 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 96cb4c6
> src/common/seconds.hpp 5ae088a
> src/common/time.hpp PRE-CREATION
> src/common/timer.hpp 71dc493
> src/common/utils.hpp 09d278a
> src/log/coordinator.hpp b881c6a
> src/log/log.hpp 79bb738
> src/log/network.hpp 9499c63
> src/master/frameworks_manager.hpp 31d708b
> src/slave/constants.hpp f0c8679
> src/slave/slave.hpp 08a29d8
> src/slave/slave.cpp 8585230
> src/state/state.hpp 6166414
> src/state/zookeeper.cpp e31fff7
> src/tests/base_zookeeper_test.hpp 2f5747e
> src/tests/slave_tests.cpp PRE-CREATION
> src/zookeeper/group.hpp 8646202
> 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
>
>
Re: Review Request: Slave Garbage Collection
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
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
>
>
Re: Review Request: Slave Garbage Collection
Posted by Vinod Kone <vi...@gmail.com>.
> On 2012-05-30 20:31:49, Benjamin Hindman wrote:
> > src/tests/slave_tests.cpp, line 68
> > <https://reviews.apache.org/r/5072/diff/5/?file=110894#file110894line68>
> >
> > Kill newline.
done
> On 2012-05-30 20:31:49, Benjamin Hindman wrote:
> > src/tests/slave_tests.cpp, line 69
> > <https://reviews.apache.org/r/5072/diff/5/?file=110894#file110894line69>
> >
> > Put '{' on newline.
oops
> On 2012-05-30 20:31:49, Benjamin Hindman wrote:
> > src/tests/slave_tests.cpp, lines 107-108
> > <https://reviews.apache.org/r/5072/diff/5/?file=110894#file110894line107>
> >
> > This has not been our style, please change.
done
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5072/#review8234
-----------------------------------------------------------
On 2012-05-30 19:45:30, Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5072/
> -----------------------------------------------------------
>
> (Updated 2012-05-30 19:45:30)
>
>
> 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 96cb4c6
> src/common/seconds.hpp 5ae088a
> src/common/time.hpp PRE-CREATION
> src/common/timer.hpp 71dc493
> src/common/utils.hpp 09d278a
> src/log/coordinator.hpp b881c6a
> src/log/log.hpp 79bb738
> src/log/network.hpp 9499c63
> src/master/frameworks_manager.hpp 31d708b
> src/slave/constants.hpp f0c8679
> src/slave/slave.hpp 08a29d8
> src/slave/slave.cpp 8585230
> src/state/state.hpp 6166414
> src/state/zookeeper.cpp e31fff7
> src/tests/base_zookeeper_test.hpp 2f5747e
> src/tests/slave_tests.cpp PRE-CREATION
> src/zookeeper/group.hpp 8646202
> 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
>
>
Re: Review Request: Slave Garbage Collection
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5072/#review8234
-----------------------------------------------------------
src/tests/slave_tests.cpp
<https://reviews.apache.org/r/5072/#comment17849>
Kill newline.
src/tests/slave_tests.cpp
<https://reviews.apache.org/r/5072/#comment17850>
Put '{' on newline.
src/tests/slave_tests.cpp
<https://reviews.apache.org/r/5072/#comment17851>
This has not been our style, please change.
- Benjamin
On 2012-05-30 19:45:30, Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5072/
> -----------------------------------------------------------
>
> (Updated 2012-05-30 19:45:30)
>
>
> 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 96cb4c6
> src/common/seconds.hpp 5ae088a
> src/common/time.hpp PRE-CREATION
> src/common/timer.hpp 71dc493
> src/common/utils.hpp 09d278a
> src/log/coordinator.hpp b881c6a
> src/log/log.hpp 79bb738
> src/log/network.hpp 9499c63
> src/master/frameworks_manager.hpp 31d708b
> src/slave/constants.hpp f0c8679
> src/slave/slave.hpp 08a29d8
> src/slave/slave.cpp 8585230
> src/state/state.hpp 6166414
> src/state/zookeeper.cpp e31fff7
> src/tests/base_zookeeper_test.hpp 2f5747e
> src/tests/slave_tests.cpp PRE-CREATION
> src/zookeeper/group.hpp 8646202
> 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
>
>
Re: Review Request: Slave Garbage Collection
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5072/#review8236
-----------------------------------------------------------
Ship it!
I'll commit this now.
- Benjamin
On 2012-05-30 20:43:29, Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5072/
> -----------------------------------------------------------
>
> (Updated 2012-05-30 20:43:29)
>
>
> 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 96cb4c6
> src/common/seconds.hpp 5ae088a
> src/common/time.hpp PRE-CREATION
> src/common/timer.hpp 71dc493
> src/common/utils.hpp 09d278a
> src/log/coordinator.hpp b881c6a
> src/log/log.hpp 79bb738
> src/log/network.hpp 9499c63
> src/master/frameworks_manager.hpp 31d708b
> src/slave/constants.hpp f0c8679
> src/slave/slave.hpp 08a29d8
> src/slave/slave.cpp 8585230
> src/state/state.hpp 6166414
> src/state/zookeeper.cpp e31fff7
> src/tests/base_zookeeper_test.hpp 2f5747e
> src/tests/slave_tests.cpp PRE-CREATION
> src/zookeeper/group.hpp 8646202
> 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
>
>
Re: Review Request: Slave Garbage Collection
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5072/#review8239
-----------------------------------------------------------
Ship it!
Thanks for the fix Vinod.
- Benjamin
On 2012-05-30 22:33:54, Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5072/
> -----------------------------------------------------------
>
> (Updated 2012-05-30 22:33:54)
>
>
> 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 96cb4c6
> src/common/seconds.hpp 5ae088a
> src/common/time.hpp PRE-CREATION
> src/common/timer.hpp 71dc493
> src/common/utils.hpp 09d278a
> src/log/coordinator.hpp b881c6a
> src/log/log.hpp 79bb738
> src/log/network.hpp 9499c63
> src/master/frameworks_manager.hpp 31d708b
> src/slave/constants.hpp f0c8679
> src/slave/slave.hpp 08a29d8
> src/slave/slave.cpp 8585230
> src/state/state.hpp 6166414
> src/state/zookeeper.cpp e31fff7
> src/tests/base_zookeeper_test.hpp 2f5747e
> src/tests/slave_tests.cpp PRE-CREATION
> src/zookeeper/group.hpp 8646202
> 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
>
>
Re: Review Request: Slave Garbage Collection
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5072/
-----------------------------------------------------------
(Updated 2012-05-31 17:09:01.016071)
Review request for mesos, Benjamin Hindman, John Sirois, and Brian Wickman.
Changes
-------
fixed slave gc tests.
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 (updated)
-----
src/Makefile.am 96cb4c6
src/common/seconds.hpp 5ae088a
src/common/time.hpp PRE-CREATION
src/common/timer.hpp 71dc493
src/common/utils.hpp 09d278a
src/log/coordinator.hpp b881c6a
src/log/log.hpp 79bb738
src/log/network.hpp 9499c63
src/master/frameworks_manager.hpp 31d708b
src/slave/constants.hpp f0c8679
src/slave/process_based_isolation_module.cpp e76339c
src/slave/slave.hpp 08a29d8
src/slave/slave.cpp 8585230
src/state/state.hpp 6166414
src/state/zookeeper.cpp e31fff7
src/tests/base_zookeeper_test.hpp 2f5747e
src/tests/slave_tests.cpp PRE-CREATION
src/zookeeper/group.hpp 8646202
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
Re: Review Request: Slave Garbage Collection
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5072/
-----------------------------------------------------------
(Updated 2012-05-30 22:33:54.134371)
Review request for mesos, Benjamin Hindman, John Sirois, and Brian Wickman.
Changes
-------
fix for test fail
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 (updated)
-----
src/Makefile.am 96cb4c6
src/common/seconds.hpp 5ae088a
src/common/time.hpp PRE-CREATION
src/common/timer.hpp 71dc493
src/common/utils.hpp 09d278a
src/log/coordinator.hpp b881c6a
src/log/log.hpp 79bb738
src/log/network.hpp 9499c63
src/master/frameworks_manager.hpp 31d708b
src/slave/constants.hpp f0c8679
src/slave/slave.hpp 08a29d8
src/slave/slave.cpp 8585230
src/state/state.hpp 6166414
src/state/zookeeper.cpp e31fff7
src/tests/base_zookeeper_test.hpp 2f5747e
src/tests/slave_tests.cpp PRE-CREATION
src/zookeeper/group.hpp 8646202
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
Re: Review Request: Slave Garbage Collection
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5072/
-----------------------------------------------------------
(Updated 2012-05-30 20:43:29.823613)
Review request for mesos, Benjamin Hindman, John Sirois, and Brian Wickman.
Changes
-------
ben's comments
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 (updated)
-----
src/Makefile.am 96cb4c6
src/common/seconds.hpp 5ae088a
src/common/time.hpp PRE-CREATION
src/common/timer.hpp 71dc493
src/common/utils.hpp 09d278a
src/log/coordinator.hpp b881c6a
src/log/log.hpp 79bb738
src/log/network.hpp 9499c63
src/master/frameworks_manager.hpp 31d708b
src/slave/constants.hpp f0c8679
src/slave/slave.hpp 08a29d8
src/slave/slave.cpp 8585230
src/state/state.hpp 6166414
src/state/zookeeper.cpp e31fff7
src/tests/base_zookeeper_test.hpp 2f5747e
src/tests/slave_tests.cpp PRE-CREATION
src/zookeeper/group.hpp 8646202
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
Re: Review Request: Slave Garbage Collection
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5072/
-----------------------------------------------------------
(Updated 2012-05-30 19:45:30.351317)
Review request for mesos, Benjamin Hindman, John Sirois, and Brian Wickman.
Changes
-------
ben's comments.
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 (updated)
-----
src/Makefile.am 96cb4c6
src/common/seconds.hpp 5ae088a
src/common/time.hpp PRE-CREATION
src/common/timer.hpp 71dc493
src/common/utils.hpp 09d278a
src/log/coordinator.hpp b881c6a
src/log/log.hpp 79bb738
src/log/network.hpp 9499c63
src/master/frameworks_manager.hpp 31d708b
src/slave/constants.hpp f0c8679
src/slave/slave.hpp 08a29d8
src/slave/slave.cpp 8585230
src/state/state.hpp 6166414
src/state/zookeeper.cpp e31fff7
src/tests/base_zookeeper_test.hpp 2f5747e
src/tests/slave_tests.cpp PRE-CREATION
src/zookeeper/group.hpp 8646202
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
Re: Review Request: Slave Garbage Collection
Posted by Benjamin Hindman <be...@berkeley.edu>.
> On 2012-05-30 17:56:07, Benjamin Hindman wrote:
> >
This looks good, but can you make the final tweaks from the comments I made to your comments from the previous round above?
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5072/#review8224
-----------------------------------------------------------
On 2012-05-30 01:33:27, Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5072/
> -----------------------------------------------------------
>
> (Updated 2012-05-30 01:33:27)
>
>
> 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 96cb4c6
> src/common/seconds.hpp 5ae088a
> src/common/time.hpp PRE-CREATION
> src/common/timer.hpp 71dc493
> src/common/utils.hpp 09d278a
> src/log/coordinator.hpp b881c6a
> src/log/log.hpp 79bb738
> src/log/network.hpp 9499c63
> src/master/frameworks_manager.hpp 31d708b
> src/slave/constants.hpp f0c8679
> src/slave/slave.hpp 08a29d8
> src/slave/slave.cpp 8585230
> src/state/state.hpp 6166414
> src/state/zookeeper.cpp e31fff7
> src/tests/base_zookeeper_test.hpp 2f5747e
> src/tests/slave_tests.cpp PRE-CREATION
> src/zookeeper/group.hpp 8646202
> 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
>
>
Re: Review Request: Slave Garbage Collection
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5072/#review8224
-----------------------------------------------------------
Ship it!
- Benjamin
On 2012-05-30 01:33:27, Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5072/
> -----------------------------------------------------------
>
> (Updated 2012-05-30 01:33:27)
>
>
> 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 96cb4c6
> src/common/seconds.hpp 5ae088a
> src/common/time.hpp PRE-CREATION
> src/common/timer.hpp 71dc493
> src/common/utils.hpp 09d278a
> src/log/coordinator.hpp b881c6a
> src/log/log.hpp 79bb738
> src/log/network.hpp 9499c63
> src/master/frameworks_manager.hpp 31d708b
> src/slave/constants.hpp f0c8679
> src/slave/slave.hpp 08a29d8
> src/slave/slave.cpp 8585230
> src/state/state.hpp 6166414
> src/state/zookeeper.cpp e31fff7
> src/tests/base_zookeeper_test.hpp 2f5747e
> src/tests/slave_tests.cpp PRE-CREATION
> src/zookeeper/group.hpp 8646202
> 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
>
>
Re: Review Request: Slave Garbage Collection
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5072/
-----------------------------------------------------------
(Updated 2012-05-30 01:33:27.527702)
Review request for mesos, Benjamin Hindman, John Sirois, and Brian Wickman.
Changes
-------
ben's comments
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 (updated)
-----
src/Makefile.am 96cb4c6
src/common/seconds.hpp 5ae088a
src/common/time.hpp PRE-CREATION
src/common/timer.hpp 71dc493
src/common/utils.hpp 09d278a
src/log/coordinator.hpp b881c6a
src/log/log.hpp 79bb738
src/log/network.hpp 9499c63
src/master/frameworks_manager.hpp 31d708b
src/slave/constants.hpp f0c8679
src/slave/slave.hpp 08a29d8
src/slave/slave.cpp 8585230
src/state/state.hpp 6166414
src/state/zookeeper.cpp e31fff7
src/tests/base_zookeeper_test.hpp 2f5747e
src/tests/slave_tests.cpp PRE-CREATION
src/zookeeper/group.hpp 8646202
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