You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Benjamin Hindman <be...@berkeley.edu> on 2012/08/10 05:00:23 UTC
Review Request: Do allocations in batch rather than reactively (i.e.,
for each event).
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6505/
-----------------------------------------------------------
Review request for mesos, Thomas Marshall, John Sirois, and Vinod Kone.
Description
-------
See summary.
Diffs
-----
src/master/allocator.hpp c6273b5
src/master/dominant_share_allocator.hpp ba5dc29
src/master/dominant_share_allocator.cpp 00a47f7
src/master/flags.hpp 1227ccc
src/master/master.cpp 4f62687
src/tests/allocator_tests.cpp b3db13d
src/tests/resource_offers_tests.cpp c004772
src/tests/utils.hpp a768360
Diff: https://reviews.apache.org/r/6505/diff/
Testing
-------
make check
Thanks,
Benjamin Hindman
Re: Review Request: Do allocations in batch rather than reactively (i.e.,
for each event).
Posted by John Sirois <jo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6505/#review10164
-----------------------------------------------------------
Ship it!
Spoke with Ben offline - the batching mechanism looks correct to me as it stands - so I won't block on a test - but it would be nice to circle back and add.
- John Sirois
On Aug. 10, 2012, 3 a.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/6505/
> -----------------------------------------------------------
>
> (Updated Aug. 10, 2012, 3 a.m.)
>
>
> Review request for mesos, Thomas Marshall, John Sirois, and Vinod Kone.
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/master/allocator.hpp c6273b5
> src/master/dominant_share_allocator.hpp ba5dc29
> src/master/dominant_share_allocator.cpp 00a47f7
> src/master/flags.hpp 1227ccc
> src/master/master.cpp 4f62687
> src/tests/allocator_tests.cpp b3db13d
> src/tests/resource_offers_tests.cpp c004772
> src/tests/utils.hpp a768360
>
> Diff: https://reviews.apache.org/r/6505/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request: Do allocations in batch rather than reactively (i.e.,
for each event).
Posted by Thomas Marshall <tw...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6505/#review10140
-----------------------------------------------------------
This patch causes some allocator tests to fail because the "real" allocator object that the MockAllocator uses to perform the actual allocation logic isn't ever getting spawned, and therefore never receives the batch() dispatches. The easiest way to fix this would be to add:
void SetUp() {
process::spawn(allocator.real)
}
void TearDown() {
process::terminate(allocator.real);
process::wait(allocator.real);
}
to the AllocatorTest class in allocator_test.cpp
- Thomas Marshall
On Aug. 10, 2012, 3 a.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/6505/
> -----------------------------------------------------------
>
> (Updated Aug. 10, 2012, 3 a.m.)
>
>
> Review request for mesos, Thomas Marshall, John Sirois, and Vinod Kone.
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/master/allocator.hpp c6273b5
> src/master/dominant_share_allocator.hpp ba5dc29
> src/master/dominant_share_allocator.cpp 00a47f7
> src/master/flags.hpp 1227ccc
> src/master/master.cpp 4f62687
> src/tests/allocator_tests.cpp b3db13d
> src/tests/resource_offers_tests.cpp c004772
> src/tests/utils.hpp a768360
>
> Diff: https://reviews.apache.org/r/6505/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request: Do allocations in batch rather than reactively (i.e.,
for each event).
Posted by John Sirois <jo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6505/#review10106
-----------------------------------------------------------
src/master/allocator.hpp
<https://reviews.apache.org/r/6505/#comment21402>
A fundamental constructor-like thing taking flags always worries me - can you have an overload - perhaps in the tested impl only - take the delay directly? I suppose you can just create a mesos master Flags and pass in - but even then only one value is needed here do mixing in a smaller Flags seems more appropriate. Either way - the new batch behavior is untested - can you leverage a Clock here? I think this concept is already covered for libprocess processes - so maybe the only thing blocking a test is someone pushing you to add one for this ;)
- John Sirois
On Aug. 10, 2012, 3 a.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/6505/
> -----------------------------------------------------------
>
> (Updated Aug. 10, 2012, 3 a.m.)
>
>
> Review request for mesos, Thomas Marshall, John Sirois, and Vinod Kone.
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/master/allocator.hpp c6273b5
> src/master/dominant_share_allocator.hpp ba5dc29
> src/master/dominant_share_allocator.cpp 00a47f7
> src/master/flags.hpp 1227ccc
> src/master/master.cpp 4f62687
> src/tests/allocator_tests.cpp b3db13d
> src/tests/resource_offers_tests.cpp c004772
> src/tests/utils.hpp a768360
>
> Diff: https://reviews.apache.org/r/6505/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request: Do allocations in batch rather than reactively (i.e.,
for each event).
Posted by Jie Yu <yu...@gmail.com>.
> On Aug. 10, 2012, 4:43 a.m., Vinod Kone wrote:
> > src/master/dominant_share_allocator.hpp, line 48
> > <https://reviews.apache.org/r/6505/diff/1/?file=136793#file136793line48>
> >
> > i thoutht we get self() for free when we inherit Process?
The self() inherited will return process::PID<Allocator>. The compiler won't allow you to pass this to a function that expect process::PID<DominantShareAllocator>.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6505/#review10108
-----------------------------------------------------------
On Aug. 10, 2012, 3 a.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/6505/
> -----------------------------------------------------------
>
> (Updated Aug. 10, 2012, 3 a.m.)
>
>
> Review request for mesos, Thomas Marshall, John Sirois, and Vinod Kone.
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/master/allocator.hpp c6273b5
> src/master/dominant_share_allocator.hpp ba5dc29
> src/master/dominant_share_allocator.cpp 00a47f7
> src/master/flags.hpp 1227ccc
> src/master/master.cpp 4f62687
> src/tests/allocator_tests.cpp b3db13d
> src/tests/resource_offers_tests.cpp c004772
> src/tests/utils.hpp a768360
>
> Diff: https://reviews.apache.org/r/6505/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request: Do allocations in batch rather than reactively (i.e.,
for each event).
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6505/#review10108
-----------------------------------------------------------
Ship it!
src/master/dominant_share_allocator.hpp
<https://reviews.apache.org/r/6505/#comment21407>
i thoutht we get self() for free when we inherit Process?
- Vinod Kone
On Aug. 10, 2012, 3 a.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/6505/
> -----------------------------------------------------------
>
> (Updated Aug. 10, 2012, 3 a.m.)
>
>
> Review request for mesos, Thomas Marshall, John Sirois, and Vinod Kone.
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/master/allocator.hpp c6273b5
> src/master/dominant_share_allocator.hpp ba5dc29
> src/master/dominant_share_allocator.cpp 00a47f7
> src/master/flags.hpp 1227ccc
> src/master/master.cpp 4f62687
> src/tests/allocator_tests.cpp b3db13d
> src/tests/resource_offers_tests.cpp c004772
> src/tests/utils.hpp a768360
>
> Diff: https://reviews.apache.org/r/6505/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>