You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Jacob Janco <jj...@gmail.com> on 2017/01/12 18:55:17 UTC
Re: Review Request 51027: Track allocation candidates to bound
allocator.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51027/
-----------------------------------------------------------
(Updated Jan. 12, 2017, 6:55 p.m.)
Review request for mesos, Benjamin Mahler, Guangya Liu, James Peach, Klaus Ma, and Jiang Yan Xu.
Changes
-------
Change JIRA.
Bugs: MESOS-6904
https://issues.apache.org/jira/browse/MESOS-6904
Repository: mesos
Description
-------
- Triggered allocations dispatch allocate() only
if there is no pending allocation in the queue.
- Allocation candidates are accumulated and only
cleared when enqueued allocations are processed.
Diffs
-----
src/master/allocator/mesos/hierarchical.hpp a6424d624864155e1c87a28a63b784512c5c8722
src/master/allocator/mesos/hierarchical.cpp 91b1ec43940a788459f045ca4a4b82d4e8373bca
Diff: https://reviews.apache.org/r/51027/diff/
Testing
-------
make check with the filters below
Broken tests:
- TEST_F(HierarchicalAllocatorTest, SuppressAndReviveOffers), fix in 51028
- TEST_F(HierarchicalAllocatorTest, AllocationRunsMetric), fix in 51028
- TEST_F(HierarchicalAllocatorTest, AllocationRunTimerMetrics), fix in 51028
- TEST_F(HierarchicalAllocatorTest, UpdateWeight), fix in 51028
- TEST_P(HierarchicalAllocator_BENCHMARK_Test, AddAndUpdateSlave), fix in 51028
- TEST_F(HierarchicalAllocatorTest, SmallOfferFilterTimeout), fix in 52534
- TEST_F(OversubscriptionTest, RescindRevocableOfferWithIncreasedRevocable), fix in 51621
Thanks,
Jacob Janco
Re: Review Request 51027: Track allocation candidates to bound
allocator.
Posted by Jacob Janco <jj...@gmail.com>.
> On Jan. 19, 2017, 2:45 a.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 1273-1280
> > <https://reviews.apache.org/r/51027/diff/11/?file=1589270#file1589270line1273>
> >
> > This change introduces an additional trip through the allocator's queue after the allocation completes and before the delay is called.
> >
> > This would avoid it:
> >
> > ```
> > auto pid = self();
> >
> > // TODO: Use process::loop for the allocation loop.
> > allocate()
> > .onAny([pid]() {
> > delay(allocationInterval, self(), &Self::batch);
> > }
> > ```
> >
> > Not sure if this was intentional or not.
This was unintentional, defer does not need to be called here.
- Jacob
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51027/#review162229
-----------------------------------------------------------
On Jan. 12, 2017, 6:55 p.m., Jacob Janco wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51027/
> -----------------------------------------------------------
>
> (Updated Jan. 12, 2017, 6:55 p.m.)
>
>
> Review request for mesos, Benjamin Mahler, Guangya Liu, James Peach, Klaus Ma, and Jiang Yan Xu.
>
>
> Bugs: MESOS-6904
> https://issues.apache.org/jira/browse/MESOS-6904
>
>
> Repository: mesos
>
>
> Description
> -------
>
> - Triggered allocations dispatch allocate() only
> if there is no pending allocation in the queue.
> - Allocation candidates are accumulated and only
> cleared when enqueued allocations are processed.
>
>
> Diffs
> -----
>
> src/master/allocator/mesos/hierarchical.hpp a6424d624864155e1c87a28a63b784512c5c8722
> src/master/allocator/mesos/hierarchical.cpp 91b1ec43940a788459f045ca4a4b82d4e8373bca
>
> Diff: https://reviews.apache.org/r/51027/diff/
>
>
> Testing
> -------
>
> make check with the filters below
>
> Broken tests:
> - TEST_F(HierarchicalAllocatorTest, SuppressAndReviveOffers), fix in 51028
> - TEST_F(HierarchicalAllocatorTest, AllocationRunsMetric), fix in 51028
> - TEST_F(HierarchicalAllocatorTest, AllocationRunTimerMetrics), fix in 51028
> - TEST_F(HierarchicalAllocatorTest, UpdateWeight), fix in 51028
> - TEST_P(HierarchicalAllocator_BENCHMARK_Test, AddAndUpdateSlave), fix in 51028
> - TEST_F(HierarchicalAllocatorTest, SmallOfferFilterTimeout), fix in 52534
> - TEST_F(OversubscriptionTest, RescindRevocableOfferWithIncreasedRevocable), fix in 51621
>
>
> Thanks,
>
> Jacob Janco
>
>
Re: Review Request 51027: Track allocation candidates to bound
allocator.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51027/#review162229
-----------------------------------------------------------
Fix it, then Ship it!
The logic looks good to me, just a few minor comments.
src/master/allocator/mesos/hierarchical.hpp (lines 221 - 229)
<https://reviews.apache.org/r/51027/#comment233533>
I think it's sufficient here to just say:
```
// Allocate resources from the specified agents.
```
The rest of the comment seems to be a re-iteration of the logic inside the implementations. Do we need to say that the other two call into this? Or how exactly we do the batching? IMHO it's likely to go stale over time and the reader can read the implementation to see exactly how it works.
We could add something like:
```
// Allocate any allocatable resources from all known agents,
// the allocation is deferred and batched with other
// allocation requests.
process::Future<Nothing> allocate();
```
But it seems unnecessary given the caller just needs to understand that it is asynchronous (i.e. returns a Future).
src/master/allocator/mesos/hierarchical.hpp (lines 232 - 236)
<https://reviews.apache.org/r/51027/#comment233536>
Run seems a bit generic. How about `_allocate()` and `__allocate()` to make it clear it's the first `_allocate()` continuation?
src/master/allocator/mesos/hierarchical.cpp (lines 1267 - 1274)
<https://reviews.apache.org/r/51027/#comment233544>
This change introduces an additional trip through the allocator's queue after the allocation completes and before the delay is called.
This would avoid it:
```
auto pid = self();
// TODO: Use process::loop for the allocation loop.
allocate()
.onAny([pid]() {
delay(allocationInterval, self(), &Self::batch);
}
```
Not sure if this was intentional or not.
src/master/allocator/mesos/hierarchical.cpp (lines 1301 - 1305)
<https://reviews.apache.org/r/51027/#comment233545>
This seems pretty self explanatory, no need for this comment IMHO. The example is likely to go stale.
src/master/allocator/mesos/hierarchical.cpp (lines 1308 - 1309)
<https://reviews.apache.org/r/51027/#comment233546>
Why the newline?
- Benjamin Mahler
On Jan. 12, 2017, 6:55 p.m., Jacob Janco wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51027/
> -----------------------------------------------------------
>
> (Updated Jan. 12, 2017, 6:55 p.m.)
>
>
> Review request for mesos, Benjamin Mahler, Guangya Liu, James Peach, Klaus Ma, and Jiang Yan Xu.
>
>
> Bugs: MESOS-6904
> https://issues.apache.org/jira/browse/MESOS-6904
>
>
> Repository: mesos
>
>
> Description
> -------
>
> - Triggered allocations dispatch allocate() only
> if there is no pending allocation in the queue.
> - Allocation candidates are accumulated and only
> cleared when enqueued allocations are processed.
>
>
> Diffs
> -----
>
> src/master/allocator/mesos/hierarchical.hpp a6424d624864155e1c87a28a63b784512c5c8722
> src/master/allocator/mesos/hierarchical.cpp 91b1ec43940a788459f045ca4a4b82d4e8373bca
>
> Diff: https://reviews.apache.org/r/51027/diff/
>
>
> Testing
> -------
>
> make check with the filters below
>
> Broken tests:
> - TEST_F(HierarchicalAllocatorTest, SuppressAndReviveOffers), fix in 51028
> - TEST_F(HierarchicalAllocatorTest, AllocationRunsMetric), fix in 51028
> - TEST_F(HierarchicalAllocatorTest, AllocationRunTimerMetrics), fix in 51028
> - TEST_F(HierarchicalAllocatorTest, UpdateWeight), fix in 51028
> - TEST_P(HierarchicalAllocator_BENCHMARK_Test, AddAndUpdateSlave), fix in 51028
> - TEST_F(HierarchicalAllocatorTest, SmallOfferFilterTimeout), fix in 52534
> - TEST_F(OversubscriptionTest, RescindRevocableOfferWithIncreasedRevocable), fix in 51621
>
>
> Thanks,
>
> Jacob Janco
>
>
Re: Review Request 51027: Track allocation candidates to bound
allocator.
Posted by Jacob Janco <jj...@gmail.com>.
> On Jan. 21, 2017, 1:24 a.m., Jiang Yan Xu wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 1314
> > <https://reviews.apache.org/r/51027/diff/11/?file=1589270#file1589270line1314>
> >
> > We can use the `|=` operator now that it's added.
Added, was waiting for that commit =D
- Jacob
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51027/#review162529
-----------------------------------------------------------
On Jan. 27, 2017, 2:33 a.m., Jacob Janco wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51027/
> -----------------------------------------------------------
>
> (Updated Jan. 27, 2017, 2:33 a.m.)
>
>
> Review request for mesos, Benjamin Mahler, Guangya Liu, James Peach, Klaus Ma, and Jiang Yan Xu.
>
>
> Bugs: MESOS-6904
> https://issues.apache.org/jira/browse/MESOS-6904
>
>
> Repository: mesos
>
>
> Description
> -------
>
> - Triggered allocations dispatch allocate() only
> if there is no pending allocation in the queue.
> - Allocation candidates are accumulated and only
> cleared when enqueued allocations are processed.
>
>
> Diffs
> -----
>
> src/master/allocator/mesos/hierarchical.hpp 9b66c23f26b37c02ed850c06c4518ea99078b02d
> src/master/allocator/mesos/hierarchical.cpp c2211be7458755aeb91ef078e4bfe92ac474044a
>
> Diff: https://reviews.apache.org/r/51027/diff/
>
>
> Testing
> -------
>
> make check with the filters below
>
> Broken tests:
> - TEST_F(HierarchicalAllocatorTest, SuppressAndReviveOffers), fix in 51028
> - TEST_F(HierarchicalAllocatorTest, AllocationRunsMetric), fix in 51028
> - TEST_F(HierarchicalAllocatorTest, AllocationRunTimerMetrics), fix in 51028
> - TEST_F(HierarchicalAllocatorTest, UpdateWeight), fix in 51028
> - TEST_P(HierarchicalAllocator_BENCHMARK_Test, AddAndUpdateSlave), fix in 51028
> - TEST_F(HierarchicalAllocatorTest, SmallOfferFilterTimeout), fix in 52534
> - TEST_F(OversubscriptionTest, RescindRevocableOfferWithIncreasedRevocable), fix in 51621
>
>
> Thanks,
>
> Jacob Janco
>
>
Re: Review Request 51027: Track allocation candidates to bound
allocator.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51027/#review162529
-----------------------------------------------------------
Ship it!
src/master/allocator/mesos/hierarchical.cpp (line 1306)
<https://reviews.apache.org/r/51027/#comment233837>
We can use the `|=` operator now that it's added.
- Jiang Yan Xu
On Jan. 12, 2017, 10:55 a.m., Jacob Janco wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51027/
> -----------------------------------------------------------
>
> (Updated Jan. 12, 2017, 10:55 a.m.)
>
>
> Review request for mesos, Benjamin Mahler, Guangya Liu, James Peach, Klaus Ma, and Jiang Yan Xu.
>
>
> Bugs: MESOS-6904
> https://issues.apache.org/jira/browse/MESOS-6904
>
>
> Repository: mesos
>
>
> Description
> -------
>
> - Triggered allocations dispatch allocate() only
> if there is no pending allocation in the queue.
> - Allocation candidates are accumulated and only
> cleared when enqueued allocations are processed.
>
>
> Diffs
> -----
>
> src/master/allocator/mesos/hierarchical.hpp a6424d624864155e1c87a28a63b784512c5c8722
> src/master/allocator/mesos/hierarchical.cpp 91b1ec43940a788459f045ca4a4b82d4e8373bca
>
> Diff: https://reviews.apache.org/r/51027/diff/
>
>
> Testing
> -------
>
> make check with the filters below
>
> Broken tests:
> - TEST_F(HierarchicalAllocatorTest, SuppressAndReviveOffers), fix in 51028
> - TEST_F(HierarchicalAllocatorTest, AllocationRunsMetric), fix in 51028
> - TEST_F(HierarchicalAllocatorTest, AllocationRunTimerMetrics), fix in 51028
> - TEST_F(HierarchicalAllocatorTest, UpdateWeight), fix in 51028
> - TEST_P(HierarchicalAllocator_BENCHMARK_Test, AddAndUpdateSlave), fix in 51028
> - TEST_F(HierarchicalAllocatorTest, SmallOfferFilterTimeout), fix in 52534
> - TEST_F(OversubscriptionTest, RescindRevocableOfferWithIncreasedRevocable), fix in 51621
>
>
> Thanks,
>
> Jacob Janco
>
>
Re: Review Request 51027: Track allocation candidates to bound
allocator.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On Feb. 3, 2017, 11:45 a.m., Benjamin Bannier wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 1263
> > <https://reviews.apache.org/r/51027/diff/14/?file=1619668#file1619668line1263>
> >
> > Could you follow up with a patch so this call is `dispatch`'ed?
> >
> > Right now, if the `HierarchicalAllocatorProcess` goes away after `allocate`, but before the `AnyCallback` finishes, `this->allocationInterval` might be referencing garbage.
>
> Jiang Yan Xu wrote:
> `delay` is already dispatched isn't it?
Yes, the `delay`'ed part is safe as it dispatches to a specific process. The issue here is with the `AnyCallBack` (the full argument to `onAny`) since it does capture a raw ptr (`this`), but is not dispatched.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51027/#review164111
-----------------------------------------------------------
On Jan. 31, 2017, 2:46 a.m., Jacob Janco wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51027/
> -----------------------------------------------------------
>
> (Updated Jan. 31, 2017, 2:46 a.m.)
>
>
> Review request for mesos, Benjamin Mahler, Guangya Liu, James Peach, Klaus Ma, and Jiang Yan Xu.
>
>
> Bugs: MESOS-6904
> https://issues.apache.org/jira/browse/MESOS-6904
>
>
> Repository: mesos
>
>
> Description
> -------
>
> - Triggered allocations dispatch allocate() only
> if there is no pending allocation in the queue.
> - Allocation candidates are accumulated and only
> cleared when enqueued allocations are processed.
>
>
> Diffs
> -----
>
> src/master/allocator/mesos/hierarchical.hpp 2cda3e311ce339d82065d68de83e86439fa192ff
> src/master/allocator/mesos/hierarchical.cpp f471b6848bebae601a7a0509e9c6ad5eab4fa4a2
>
> Diff: https://reviews.apache.org/r/51027/diff/
>
>
> Testing
> -------
>
> make check with the filters below
>
> Broken tests:
> - TEST_F(HierarchicalAllocatorTest, SuppressAndReviveOffers), fix in 51028
> - TEST_F(HierarchicalAllocatorTest, AllocationRunsMetric), fix in 51028
> - TEST_F(HierarchicalAllocatorTest, AllocationRunTimerMetrics), fix in 51028
> - TEST_F(HierarchicalAllocatorTest, UpdateWeight), fix in 51028
> - TEST_P(HierarchicalAllocator_BENCHMARK_Test, AddAndUpdateSlave), fix in 51028
> - TEST_F(HierarchicalAllocatorTest, SmallOfferFilterTimeout), fix in 52534
> - TEST_F(OversubscriptionTest, RescindRevocableOfferWithIncreasedRevocable), fix in 51621
>
>
> Thanks,
>
> Jacob Janco
>
>
Re: Review Request 51027: Track allocation candidates to bound
allocator.
Posted by Jacob Janco <jj...@gmail.com>.
> On Feb. 3, 2017, 10:45 a.m., Benjamin Bannier wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 1263
> > <https://reviews.apache.org/r/51027/diff/14/?file=1619668#file1619668line1263>
> >
> > Could you follow up with a patch so this call is `dispatch`'ed?
> >
> > Right now, if the `HierarchicalAllocatorProcess` goes away after `allocate`, but before the `AnyCallback` finishes, `this->allocationInterval` might be referencing garbage.
>
> Jiang Yan Xu wrote:
> `delay` is already dispatched isn't it?
>
> Benjamin Bannier wrote:
> Yes, the `delay`'ed part is safe as it dispatches to a specific process. The issue here is with the `AnyCallBack` (the full argument to `onAny`) since it does capture a raw ptr (`this`), but is not dispatched.
>
> Jiang Yan Xu wrote:
> Ah I see what you mean now, thanks! I guess we can copy `allocationInterval` in.
>
> Jacob Janco wrote:
> ^I'll submit a patch with the above fix.
https://reviews.apache.org/r/56296
- Jacob
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51027/#review164111
-----------------------------------------------------------
On Jan. 31, 2017, 1:46 a.m., Jacob Janco wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51027/
> -----------------------------------------------------------
>
> (Updated Jan. 31, 2017, 1:46 a.m.)
>
>
> Review request for mesos, Benjamin Mahler, Guangya Liu, James Peach, Klaus Ma, and Jiang Yan Xu.
>
>
> Bugs: MESOS-6904
> https://issues.apache.org/jira/browse/MESOS-6904
>
>
> Repository: mesos
>
>
> Description
> -------
>
> - Triggered allocations dispatch allocate() only
> if there is no pending allocation in the queue.
> - Allocation candidates are accumulated and only
> cleared when enqueued allocations are processed.
>
>
> Diffs
> -----
>
> src/master/allocator/mesos/hierarchical.hpp 2cda3e311ce339d82065d68de83e86439fa192ff
> src/master/allocator/mesos/hierarchical.cpp f471b6848bebae601a7a0509e9c6ad5eab4fa4a2
>
> Diff: https://reviews.apache.org/r/51027/diff/
>
>
> Testing
> -------
>
> make check with the filters below
>
> Broken tests:
> - TEST_F(HierarchicalAllocatorTest, SuppressAndReviveOffers), fix in 51028
> - TEST_F(HierarchicalAllocatorTest, AllocationRunsMetric), fix in 51028
> - TEST_F(HierarchicalAllocatorTest, AllocationRunTimerMetrics), fix in 51028
> - TEST_F(HierarchicalAllocatorTest, UpdateWeight), fix in 51028
> - TEST_P(HierarchicalAllocator_BENCHMARK_Test, AddAndUpdateSlave), fix in 51028
> - TEST_F(HierarchicalAllocatorTest, SmallOfferFilterTimeout), fix in 52534
> - TEST_F(OversubscriptionTest, RescindRevocableOfferWithIncreasedRevocable), fix in 51621
>
>
> Thanks,
>
> Jacob Janco
>
>
Re: Review Request 51027: Track allocation candidates to bound
allocator.
Posted by Jiang Yan Xu <xu...@apple.com>.
> On Feb. 3, 2017, 2:45 a.m., Benjamin Bannier wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 1263
> > <https://reviews.apache.org/r/51027/diff/14/?file=1619668#file1619668line1263>
> >
> > Could you follow up with a patch so this call is `dispatch`'ed?
> >
> > Right now, if the `HierarchicalAllocatorProcess` goes away after `allocate`, but before the `AnyCallback` finishes, `this->allocationInterval` might be referencing garbage.
>
> Jiang Yan Xu wrote:
> `delay` is already dispatched isn't it?
>
> Benjamin Bannier wrote:
> Yes, the `delay`'ed part is safe as it dispatches to a specific process. The issue here is with the `AnyCallBack` (the full argument to `onAny`) since it does capture a raw ptr (`this`), but is not dispatched.
Ah I see what you mean now, thanks! I guess we can copy `allocationInterval` in.
- Jiang Yan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51027/#review164111
-----------------------------------------------------------
On Jan. 30, 2017, 5:46 p.m., Jacob Janco wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51027/
> -----------------------------------------------------------
>
> (Updated Jan. 30, 2017, 5:46 p.m.)
>
>
> Review request for mesos, Benjamin Mahler, Guangya Liu, James Peach, Klaus Ma, and Jiang Yan Xu.
>
>
> Bugs: MESOS-6904
> https://issues.apache.org/jira/browse/MESOS-6904
>
>
> Repository: mesos
>
>
> Description
> -------
>
> - Triggered allocations dispatch allocate() only
> if there is no pending allocation in the queue.
> - Allocation candidates are accumulated and only
> cleared when enqueued allocations are processed.
>
>
> Diffs
> -----
>
> src/master/allocator/mesos/hierarchical.hpp 2cda3e311ce339d82065d68de83e86439fa192ff
> src/master/allocator/mesos/hierarchical.cpp f471b6848bebae601a7a0509e9c6ad5eab4fa4a2
>
> Diff: https://reviews.apache.org/r/51027/diff/
>
>
> Testing
> -------
>
> make check with the filters below
>
> Broken tests:
> - TEST_F(HierarchicalAllocatorTest, SuppressAndReviveOffers), fix in 51028
> - TEST_F(HierarchicalAllocatorTest, AllocationRunsMetric), fix in 51028
> - TEST_F(HierarchicalAllocatorTest, AllocationRunTimerMetrics), fix in 51028
> - TEST_F(HierarchicalAllocatorTest, UpdateWeight), fix in 51028
> - TEST_P(HierarchicalAllocator_BENCHMARK_Test, AddAndUpdateSlave), fix in 51028
> - TEST_F(HierarchicalAllocatorTest, SmallOfferFilterTimeout), fix in 52534
> - TEST_F(OversubscriptionTest, RescindRevocableOfferWithIncreasedRevocable), fix in 51621
>
>
> Thanks,
>
> Jacob Janco
>
>
Re: Review Request 51027: Track allocation candidates to bound
allocator.
Posted by Jiang Yan Xu <xu...@apple.com>.
> On Feb. 3, 2017, 2:45 a.m., Benjamin Bannier wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 1263
> > <https://reviews.apache.org/r/51027/diff/14/?file=1619668#file1619668line1263>
> >
> > Could you follow up with a patch so this call is `dispatch`'ed?
> >
> > Right now, if the `HierarchicalAllocatorProcess` goes away after `allocate`, but before the `AnyCallback` finishes, `this->allocationInterval` might be referencing garbage.
`delay` is already dispatched isn't it?
- Jiang Yan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51027/#review164111
-----------------------------------------------------------
On Jan. 30, 2017, 5:46 p.m., Jacob Janco wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51027/
> -----------------------------------------------------------
>
> (Updated Jan. 30, 2017, 5:46 p.m.)
>
>
> Review request for mesos, Benjamin Mahler, Guangya Liu, James Peach, Klaus Ma, and Jiang Yan Xu.
>
>
> Bugs: MESOS-6904
> https://issues.apache.org/jira/browse/MESOS-6904
>
>
> Repository: mesos
>
>
> Description
> -------
>
> - Triggered allocations dispatch allocate() only
> if there is no pending allocation in the queue.
> - Allocation candidates are accumulated and only
> cleared when enqueued allocations are processed.
>
>
> Diffs
> -----
>
> src/master/allocator/mesos/hierarchical.hpp 2cda3e311ce339d82065d68de83e86439fa192ff
> src/master/allocator/mesos/hierarchical.cpp f471b6848bebae601a7a0509e9c6ad5eab4fa4a2
>
> Diff: https://reviews.apache.org/r/51027/diff/
>
>
> Testing
> -------
>
> make check with the filters below
>
> Broken tests:
> - TEST_F(HierarchicalAllocatorTest, SuppressAndReviveOffers), fix in 51028
> - TEST_F(HierarchicalAllocatorTest, AllocationRunsMetric), fix in 51028
> - TEST_F(HierarchicalAllocatorTest, AllocationRunTimerMetrics), fix in 51028
> - TEST_F(HierarchicalAllocatorTest, UpdateWeight), fix in 51028
> - TEST_P(HierarchicalAllocator_BENCHMARK_Test, AddAndUpdateSlave), fix in 51028
> - TEST_F(HierarchicalAllocatorTest, SmallOfferFilterTimeout), fix in 52534
> - TEST_F(OversubscriptionTest, RescindRevocableOfferWithIncreasedRevocable), fix in 51621
>
>
> Thanks,
>
> Jacob Janco
>
>
Re: Review Request 51027: Track allocation candidates to bound
allocator.
Posted by Jacob Janco <jj...@gmail.com>.
> On Feb. 3, 2017, 10:45 a.m., Benjamin Bannier wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 1263
> > <https://reviews.apache.org/r/51027/diff/14/?file=1619668#file1619668line1263>
> >
> > Could you follow up with a patch so this call is `dispatch`'ed?
> >
> > Right now, if the `HierarchicalAllocatorProcess` goes away after `allocate`, but before the `AnyCallback` finishes, `this->allocationInterval` might be referencing garbage.
>
> Jiang Yan Xu wrote:
> `delay` is already dispatched isn't it?
>
> Benjamin Bannier wrote:
> Yes, the `delay`'ed part is safe as it dispatches to a specific process. The issue here is with the `AnyCallBack` (the full argument to `onAny`) since it does capture a raw ptr (`this`), but is not dispatched.
>
> Jiang Yan Xu wrote:
> Ah I see what you mean now, thanks! I guess we can copy `allocationInterval` in.
^I'll submit a patch with the above fix.
- Jacob
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51027/#review164111
-----------------------------------------------------------
On Jan. 31, 2017, 1:46 a.m., Jacob Janco wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51027/
> -----------------------------------------------------------
>
> (Updated Jan. 31, 2017, 1:46 a.m.)
>
>
> Review request for mesos, Benjamin Mahler, Guangya Liu, James Peach, Klaus Ma, and Jiang Yan Xu.
>
>
> Bugs: MESOS-6904
> https://issues.apache.org/jira/browse/MESOS-6904
>
>
> Repository: mesos
>
>
> Description
> -------
>
> - Triggered allocations dispatch allocate() only
> if there is no pending allocation in the queue.
> - Allocation candidates are accumulated and only
> cleared when enqueued allocations are processed.
>
>
> Diffs
> -----
>
> src/master/allocator/mesos/hierarchical.hpp 2cda3e311ce339d82065d68de83e86439fa192ff
> src/master/allocator/mesos/hierarchical.cpp f471b6848bebae601a7a0509e9c6ad5eab4fa4a2
>
> Diff: https://reviews.apache.org/r/51027/diff/
>
>
> Testing
> -------
>
> make check with the filters below
>
> Broken tests:
> - TEST_F(HierarchicalAllocatorTest, SuppressAndReviveOffers), fix in 51028
> - TEST_F(HierarchicalAllocatorTest, AllocationRunsMetric), fix in 51028
> - TEST_F(HierarchicalAllocatorTest, AllocationRunTimerMetrics), fix in 51028
> - TEST_F(HierarchicalAllocatorTest, UpdateWeight), fix in 51028
> - TEST_P(HierarchicalAllocator_BENCHMARK_Test, AddAndUpdateSlave), fix in 51028
> - TEST_F(HierarchicalAllocatorTest, SmallOfferFilterTimeout), fix in 52534
> - TEST_F(OversubscriptionTest, RescindRevocableOfferWithIncreasedRevocable), fix in 51621
>
>
> Thanks,
>
> Jacob Janco
>
>
Re: Review Request 51027: Track allocation candidates to bound
allocator.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51027/#review164111
-----------------------------------------------------------
src/master/allocator/mesos/hierarchical.cpp (line 1257)
<https://reviews.apache.org/r/51027/#comment235691>
Could you follow up with a patch so this call is `dispatch`'ed?
Right now, if the `HierarchicalAllocatorProcess` goes away after `allocate`, but before the `AnyCallback` finishes, `this->allocationInterval` might be referencing garbage.
- Benjamin Bannier
On Jan. 31, 2017, 2:46 a.m., Jacob Janco wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51027/
> -----------------------------------------------------------
>
> (Updated Jan. 31, 2017, 2:46 a.m.)
>
>
> Review request for mesos, Benjamin Mahler, Guangya Liu, James Peach, Klaus Ma, and Jiang Yan Xu.
>
>
> Bugs: MESOS-6904
> https://issues.apache.org/jira/browse/MESOS-6904
>
>
> Repository: mesos
>
>
> Description
> -------
>
> - Triggered allocations dispatch allocate() only
> if there is no pending allocation in the queue.
> - Allocation candidates are accumulated and only
> cleared when enqueued allocations are processed.
>
>
> Diffs
> -----
>
> src/master/allocator/mesos/hierarchical.hpp 2cda3e311ce339d82065d68de83e86439fa192ff
> src/master/allocator/mesos/hierarchical.cpp f471b6848bebae601a7a0509e9c6ad5eab4fa4a2
>
> Diff: https://reviews.apache.org/r/51027/diff/
>
>
> Testing
> -------
>
> make check with the filters below
>
> Broken tests:
> - TEST_F(HierarchicalAllocatorTest, SuppressAndReviveOffers), fix in 51028
> - TEST_F(HierarchicalAllocatorTest, AllocationRunsMetric), fix in 51028
> - TEST_F(HierarchicalAllocatorTest, AllocationRunTimerMetrics), fix in 51028
> - TEST_F(HierarchicalAllocatorTest, UpdateWeight), fix in 51028
> - TEST_P(HierarchicalAllocator_BENCHMARK_Test, AddAndUpdateSlave), fix in 51028
> - TEST_F(HierarchicalAllocatorTest, SmallOfferFilterTimeout), fix in 52534
> - TEST_F(OversubscriptionTest, RescindRevocableOfferWithIncreasedRevocable), fix in 51621
>
>
> Thanks,
>
> Jacob Janco
>
>
Re: Review Request 51027: Track allocation candidates to bound
allocator.
Posted by Jacob Janco <jj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51027/
-----------------------------------------------------------
(Updated Jan. 31, 2017, 1:46 a.m.)
Review request for mesos, Benjamin Mahler, Guangya Liu, James Peach, Klaus Ma, and Jiang Yan Xu.
Changes
-------
Small fix to batch allocation.
Bugs: MESOS-6904
https://issues.apache.org/jira/browse/MESOS-6904
Repository: mesos
Description
-------
- Triggered allocations dispatch allocate() only
if there is no pending allocation in the queue.
- Allocation candidates are accumulated and only
cleared when enqueued allocations are processed.
Diffs (updated)
-----
src/master/allocator/mesos/hierarchical.hpp 2cda3e311ce339d82065d68de83e86439fa192ff
src/master/allocator/mesos/hierarchical.cpp f471b6848bebae601a7a0509e9c6ad5eab4fa4a2
Diff: https://reviews.apache.org/r/51027/diff/
Testing
-------
make check with the filters below
Broken tests:
- TEST_F(HierarchicalAllocatorTest, SuppressAndReviveOffers), fix in 51028
- TEST_F(HierarchicalAllocatorTest, AllocationRunsMetric), fix in 51028
- TEST_F(HierarchicalAllocatorTest, AllocationRunTimerMetrics), fix in 51028
- TEST_F(HierarchicalAllocatorTest, UpdateWeight), fix in 51028
- TEST_P(HierarchicalAllocator_BENCHMARK_Test, AddAndUpdateSlave), fix in 51028
- TEST_F(HierarchicalAllocatorTest, SmallOfferFilterTimeout), fix in 52534
- TEST_F(OversubscriptionTest, RescindRevocableOfferWithIncreasedRevocable), fix in 51621
Thanks,
Jacob Janco
Re: Review Request 51027: Track allocation candidates to bound
allocator.
Posted by Jacob Janco <jj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51027/
-----------------------------------------------------------
(Updated Jan. 31, 2017, 1:41 a.m.)
Review request for mesos, Benjamin Mahler, Guangya Liu, James Peach, Klaus Ma, and Jiang Yan Xu.
Changes
-------
Rebase.
Bugs: MESOS-6904
https://issues.apache.org/jira/browse/MESOS-6904
Repository: mesos
Description
-------
- Triggered allocations dispatch allocate() only
if there is no pending allocation in the queue.
- Allocation candidates are accumulated and only
cleared when enqueued allocations are processed.
Diffs (updated)
-----
src/master/allocator/mesos/hierarchical.hpp 2cda3e311ce339d82065d68de83e86439fa192ff
src/master/allocator/mesos/hierarchical.cpp f471b6848bebae601a7a0509e9c6ad5eab4fa4a2
Diff: https://reviews.apache.org/r/51027/diff/
Testing
-------
make check with the filters below
Broken tests:
- TEST_F(HierarchicalAllocatorTest, SuppressAndReviveOffers), fix in 51028
- TEST_F(HierarchicalAllocatorTest, AllocationRunsMetric), fix in 51028
- TEST_F(HierarchicalAllocatorTest, AllocationRunTimerMetrics), fix in 51028
- TEST_F(HierarchicalAllocatorTest, UpdateWeight), fix in 51028
- TEST_P(HierarchicalAllocator_BENCHMARK_Test, AddAndUpdateSlave), fix in 51028
- TEST_F(HierarchicalAllocatorTest, SmallOfferFilterTimeout), fix in 52534
- TEST_F(OversubscriptionTest, RescindRevocableOfferWithIncreasedRevocable), fix in 51621
Thanks,
Jacob Janco
Re: Review Request 51027: Track allocation candidates to bound
allocator.
Posted by Jiang Yan Xu <xu...@apple.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51027/#review163610
-----------------------------------------------------------
Fix it, then Ship it!
src/master/allocator/mesos/hierarchical.cpp (lines 1260 - 1262)
<https://reviews.apache.org/r/51027/#comment235078>
`onAny` doesn't require this. (And we actually don't need `then()` here as we don't actually return anything. I was wrong to suggest it previously. :)
```
allocate()
.onAny([pid, this]() {
delay(allocationInterval, pid, &Self::batch);
});
```
should just work.
- Jiang Yan Xu
On Jan. 26, 2017, 6:33 p.m., Jacob Janco wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51027/
> -----------------------------------------------------------
>
> (Updated Jan. 26, 2017, 6:33 p.m.)
>
>
> Review request for mesos, Benjamin Mahler, Guangya Liu, James Peach, Klaus Ma, and Jiang Yan Xu.
>
>
> Bugs: MESOS-6904
> https://issues.apache.org/jira/browse/MESOS-6904
>
>
> Repository: mesos
>
>
> Description
> -------
>
> - Triggered allocations dispatch allocate() only
> if there is no pending allocation in the queue.
> - Allocation candidates are accumulated and only
> cleared when enqueued allocations are processed.
>
>
> Diffs
> -----
>
> src/master/allocator/mesos/hierarchical.hpp 9b66c23f26b37c02ed850c06c4518ea99078b02d
> src/master/allocator/mesos/hierarchical.cpp c2211be7458755aeb91ef078e4bfe92ac474044a
>
> Diff: https://reviews.apache.org/r/51027/diff/
>
>
> Testing
> -------
>
> make check with the filters below
>
> Broken tests:
> - TEST_F(HierarchicalAllocatorTest, SuppressAndReviveOffers), fix in 51028
> - TEST_F(HierarchicalAllocatorTest, AllocationRunsMetric), fix in 51028
> - TEST_F(HierarchicalAllocatorTest, AllocationRunTimerMetrics), fix in 51028
> - TEST_F(HierarchicalAllocatorTest, UpdateWeight), fix in 51028
> - TEST_P(HierarchicalAllocator_BENCHMARK_Test, AddAndUpdateSlave), fix in 51028
> - TEST_F(HierarchicalAllocatorTest, SmallOfferFilterTimeout), fix in 52534
> - TEST_F(OversubscriptionTest, RescindRevocableOfferWithIncreasedRevocable), fix in 51621
>
>
> Thanks,
>
> Jacob Janco
>
>
Re: Review Request 51027: Track allocation candidates to bound
allocator.
Posted by Jacob Janco <jj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51027/
-----------------------------------------------------------
(Updated Jan. 27, 2017, 2:33 a.m.)
Review request for mesos, Benjamin Mahler, Guangya Liu, James Peach, Klaus Ma, and Jiang Yan Xu.
Bugs: MESOS-6904
https://issues.apache.org/jira/browse/MESOS-6904
Repository: mesos
Description
-------
- Triggered allocations dispatch allocate() only
if there is no pending allocation in the queue.
- Allocation candidates are accumulated and only
cleared when enqueued allocations are processed.
Diffs (updated)
-----
src/master/allocator/mesos/hierarchical.hpp 9b66c23f26b37c02ed850c06c4518ea99078b02d
src/master/allocator/mesos/hierarchical.cpp c2211be7458755aeb91ef078e4bfe92ac474044a
Diff: https://reviews.apache.org/r/51027/diff/
Testing
-------
make check with the filters below
Broken tests:
- TEST_F(HierarchicalAllocatorTest, SuppressAndReviveOffers), fix in 51028
- TEST_F(HierarchicalAllocatorTest, AllocationRunsMetric), fix in 51028
- TEST_F(HierarchicalAllocatorTest, AllocationRunTimerMetrics), fix in 51028
- TEST_F(HierarchicalAllocatorTest, UpdateWeight), fix in 51028
- TEST_P(HierarchicalAllocator_BENCHMARK_Test, AddAndUpdateSlave), fix in 51028
- TEST_F(HierarchicalAllocatorTest, SmallOfferFilterTimeout), fix in 52534
- TEST_F(OversubscriptionTest, RescindRevocableOfferWithIncreasedRevocable), fix in 51621
Thanks,
Jacob Janco