Re: Review Request 51027: Track allocation candidates to bound allocator.

[Jacob Janco]

> On Feb. 3, 2017, 10:45 a.m., Benjamin Bannier wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 1263
> > 
> >
> > 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.

[Jacob Janco]

> On Feb. 3, 2017, 10:45 a.m., Benjamin Bannier wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 1263
> > 
> >
> > 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.

[Jiang Yan Xu]

> On Feb. 3, 2017, 2:45 a.m., Benjamin Bannier wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 1263
> > 
> >
> > 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.

[Benjamin Bannier]

> On Feb. 3, 2017, 11:45 a.m., Benjamin Bannier wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 1263
> > 
> >
> > 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.

[Jiang Yan Xu]

> On Feb. 3, 2017, 2:45 a.m., Benjamin Bannier wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 1263
> > 
> >
> > 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.

[Benjamin Bannier]

---
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)


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.

[Jacob Janco]

---
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.

[Jacob Janco]

---
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.

[Jiang Yan Xu]

---
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)


`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, ::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.

[Jacob Janco]

> On Jan. 21, 2017, 1:24 a.m., Jiang Yan Xu wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 1314
> > 
> >
> > 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.

[Jacob Janco]

---
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

Re: Review Request 51027: Track allocation candidates to bound allocator.

[Jacob Janco]

> On Jan. 19, 2017, 2:45 a.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 1273-1280
> > 
> >
> > 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(), ::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.

[Jiang Yan Xu]

---
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)


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.

[Benjamin Mahler]

---
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)


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 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)


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)


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(), ::batch);
  }
```

Not sure if this was intentional or not.



src/master/allocator/mesos/hierarchical.cpp (lines 1301 - 1305)


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)


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.

[Jacob Janco]

---
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.

[Jacob Janco]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51027/
---

(Updated Dec. 21, 2016, 6:02 a.m.)


Review request for mesos, Benjamin Mahler, Guangya Liu, James Peach, Klaus Ma, 
and Jiang Yan Xu.


Bugs: MESOS-3157
https://issues.apache.org/jira/browse/MESOS-3157


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.

[Jacob Janco]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51027/
---

(Updated Dec. 20, 2016, 7:35 a.m.)


Review request for mesos, Benjamin Mahler, Guangya Liu, James Peach, Klaus Ma, 
and Jiang Yan Xu.


Bugs: MESOS-3157
https://issues.apache.org/jira/browse/MESOS-3157


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 (updated)
---

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.

[Jacob Janco]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51027/
---

(Updated Dec. 20, 2016, 7:20 a.m.)


Review request for mesos, Benjamin Mahler, Guangya Liu, James Peach, Klaus Ma, 
and Jiang Yan Xu.


Bugs: MESOS-3157
https://issues.apache.org/jira/browse/MESOS-3157


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 
a6424d624864155e1c87a28a63b784512c5c8722 
  src/master/allocator/mesos/hierarchical.cpp 
91b1ec43940a788459f045ca4a4b82d4e8373bca 

Diff: https://reviews.apache.org/r/51027/diff/


Testing
---

make check

note: check without filters depends on https://reviews.apache.org/r/51028 and 
https://reviews.apache.org/r/52534


Thanks,

Jacob Janco

Re: Review Request 51027: Track allocation candidates to bound allocator.

[Jacob Janco]

> On Dec. 12, 2016, 9:30 a.m., Jiang Yan Xu wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 1307
> > 
> >
> > s/a single slaveId/a single `slaveId`/ to be consistent.

Changed the wording to describe the events in plain english, previously I was 
describing methods. 

  // Events that trigger allocations, e.g. adding an agent or a framework,
  // update the set of `allocationCandidates` with a single agent in the
  // former case and the set of all known agents in the latter.


> On Dec. 12, 2016, 9:30 a.m., Jiang Yan Xu wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 1287-1288
> > 
> >
> > Does this work?
> > 
> > ```
> > return allocate({slaveId});
> > ```

No, the hashset constructor allows an initializer list, I think `return 
allocate({slaveId});` would just scope `slaveId`.


- Jacob


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51027/#review158818
---


On Oct. 18, 2016, 4:14 p.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51027/
> ---
> 
> (Updated Oct. 18, 2016, 4:14 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Guangya Liu, James Peach, Klaus 
> Ma, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-3157
> https://issues.apache.org/jira/browse/MESOS-3157
> 
> 
> 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 
> 2c31471ee0f5d6836393bf87ff9ecfd8df835013 
>   src/master/allocator/mesos/hierarchical.cpp 
> c8f9492ee1b69e125a1e841116d22a578a9b524e 
> 
> Diff: https://reviews.apache.org/r/51027/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> note: check without filters depends on https://reviews.apache.org/r/51028 and 
> https://reviews.apache.org/r/52534
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

Re: Review Request 51027: Track allocation candidates to bound allocator.

[Jiang Yan Xu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51027/#review158818
---



This patch itself LGTM. Will give ship it once we confirm the flaky tests are 
fixed and we have at least minimal test for its efficacy.


src/master/allocator/mesos/hierarchical.hpp (line 224)


To make it more clear to the reader, we can explain what the future means:

```
The returned future becomes ready when the latest allocation run which 
commences after this call has completed.

NOTE: At any given moment there's at most one allocation run pending or 
being executed.
```



src/master/allocator/mesos/hierarchical.hpp (line 227)


The following should be sufficient.

```
// Method that performs the allocation work.
```



src/master/allocator/mesos/hierarchical.cpp (lines 1281 - 1282)


Does this work?

```
return allocate({slaveId});
```



src/master/allocator/mesos/hierarchical.cpp (lines 1297 - 1298)


"allocation-triggering events" better than "events triggering allocations"?



src/master/allocator/mesos/hierarchical.cpp (line 1299)


s/a single slaveId/a single `slaveId`/ to be consistent.



src/master/allocator/mesos/hierarchical.cpp (lines 1334 - 1338)


Clear the candidates after the log line because you need 
`allocationCandidates.size()`?



src/master/allocator/mesos/hierarchical.cpp (line 1347)


Move this to `run()`, above `Stopwatch stopwatch;`.


- Jiang Yan Xu


On Oct. 18, 2016, 9:14 a.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51027/
> ---
> 
> (Updated Oct. 18, 2016, 9:14 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Guangya Liu, James Peach, Klaus 
> Ma, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-3157
> https://issues.apache.org/jira/browse/MESOS-3157
> 
> 
> 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 
> 2c31471ee0f5d6836393bf87ff9ecfd8df835013 
>   src/master/allocator/mesos/hierarchical.cpp 
> c8f9492ee1b69e125a1e841116d22a578a9b524e 
> 
> Diff: https://reviews.apache.org/r/51027/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> note: check without filters depends on https://reviews.apache.org/r/51028 and 
> https://reviews.apache.org/r/52534
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

Re: Review Request 51027: Track allocation candidates to bound allocator.

[Guangya Liu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51027/#review153345
---



Jacob, regaring the test failure of 
`OversubscriptionTest.RescindRevocableOfferWithIncreasedRevocable`, I think 
that you can disable it as 
`DISABLED_RescindRevocableOfferWithIncreasedRevocable` in your patch, and I 
will fix this in /r/51621

I found that with current logic, it is difficult to handle this case, as we 
cannot make sure the complete order for `allocator->updateSlave` and 
`allocator->recoverResources`, if `allocator->recoverResources` finished first, 
then the offer will have `3 REV resources`; if `allocator->updateSlave` 
finished first, then the offer will have `2 REV resources`.

With /r/51621 , after we enable `recoverResources` allocate resources, we can 
always make sure we get `3 REV resources` in the offer, so I propose that you 
disable the test case first and I will fix this after we enable allocate 
resources when `recoverResoures`.

- Guangya Liu


On 十月 18, 2016, 4:14 p.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51027/
> ---
> 
> (Updated 十月 18, 2016, 4:14 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Guangya Liu, James Peach, Klaus 
> Ma, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-3157
> https://issues.apache.org/jira/browse/MESOS-3157
> 
> 
> 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 
> 2c31471ee0f5d6836393bf87ff9ecfd8df835013 
>   src/master/allocator/mesos/hierarchical.cpp 
> c8f9492ee1b69e125a1e841116d22a578a9b524e 
> 
> Diff: https://reviews.apache.org/r/51027/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> note: check without filters depends on https://reviews.apache.org/r/51028 and 
> https://reviews.apache.org/r/52534
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

Re: Review Request 51027: Track allocation candidates to bound allocator.

[Jacob Janco]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51027/
---

(Updated Oct. 18, 2016, 4:14 p.m.)


Review request for mesos, Benjamin Mahler, Guangya Liu, James Peach, Klaus Ma, 
and Jiang Yan Xu.


Bugs: MESOS-3157
https://issues.apache.org/jira/browse/MESOS-3157


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 
2c31471ee0f5d6836393bf87ff9ecfd8df835013 
  src/master/allocator/mesos/hierarchical.cpp 
c8f9492ee1b69e125a1e841116d22a578a9b524e 

Diff: https://reviews.apache.org/r/51027/diff/


Testing
---

make check

note: check without filters depends on https://reviews.apache.org/r/51028 and 
https://reviews.apache.org/r/52534


Thanks,

Jacob Janco

Re: Review Request 51027: Track allocation candidates to bound allocator.

[Guangya Liu]

> On 十月 4, 2016, 9:47 p.m., Guangya Liu wrote:
> > Jacob, two other comments for this:
> > 
> > 1) when will you have the patch `SmallOfferFilter` ready for review?
> > 2) I think we may need another benchmark test which only do two operations: 
> > a) add multiple agents first 2) add multiple frameworks, this test can make 
> > sure we `batched` the `allocate()` request for all frameworks.
> 
> Jacob Janco wrote:
> Added the fix for you: https://reviews.apache.org/r/52534

Are you planning to add a new benchmark test of add agents first then add 
frameworks to test your patch?


> On 十月 4, 2016, 9:47 p.m., Guangya Liu wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 1306
> > 
> >
> > The `addFramework` will call `allocate()` and allocate `all agents 
> > resources` but not a `single agnent`, so here we should not mention 
> > `addFramework`, but only the following three:
> > 
> > 1) addSlave
> > 2) updateSlave
> > 3) updateUnavailability?
> 
> Jacob Janco wrote:
> I mentioend addFramework and addSlave intentionally so we have an example 
> of allocating all known slaves as well as for a single slave. In both cases 
> we will take the union of allocationCandidates. For example I 
> addSlave(agent1) and immediately addFramework() ... an allocation would be 
> queued for the single agent then all agents would be added in the union when 
> addFramework() is called. When the actual allocation run occurs all slaves 
> known at the time of addFramework() will be included in allocationCandidates.

That make sense, thanks Jacob.


- Guangya


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51027/#review151408
---


On 十月 4, 2016, 11:31 p.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51027/
> ---
> 
> (Updated 十月 4, 2016, 11:31 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Guangya Liu, James Peach, Klaus 
> Ma, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-3157
> https://issues.apache.org/jira/browse/MESOS-3157
> 
> 
> 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 
> 2c31471ee0f5d6836393bf87ff9ecfd8df835013 
>   src/master/allocator/mesos/hierarchical.cpp 
> c8f9492ee1b69e125a1e841116d22a578a9b524e 
> 
> Diff: https://reviews.apache.org/r/51027/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> note: check without filters depends on https://reviews.apache.org/r/51028 and 
> https://reviews.apache.org/r/52534
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

Re: Review Request 51027: Track allocation candidates to bound allocator.

[Jacob Janco]

> On Oct. 4, 2016, 9:47 p.m., Guangya Liu wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 1306
> > 
> >
> > The `addFramework` will call `allocate()` and allocate `all agents 
> > resources` but not a `single agnent`, so here we should not mention 
> > `addFramework`, but only the following three:
> > 
> > 1) addSlave
> > 2) updateSlave
> > 3) updateUnavailability?

I mentioend addFramework and addSlave intentionally so we have an example of 
allocating all known slaves as well as for a single slave. In both cases we 
will take the union of allocationCandidates. For example I addSlave(agent1) and 
immediately addFramework() ... an allocation would be queued for the single 
agent then all agents would be added in the union when addFramework() is 
called. When the actual allocation run occurs all slaves known at the time of 
addFramework() will be included in allocationCandidates.


- Jacob


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51027/#review151408
---


On Oct. 4, 2016, 11:31 p.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51027/
> ---
> 
> (Updated Oct. 4, 2016, 11:31 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Guangya Liu, James Peach, Klaus 
> Ma, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-3157
> https://issues.apache.org/jira/browse/MESOS-3157
> 
> 
> 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 
> 2c31471ee0f5d6836393bf87ff9ecfd8df835013 
>   src/master/allocator/mesos/hierarchical.cpp 
> c8f9492ee1b69e125a1e841116d22a578a9b524e 
> 
> Diff: https://reviews.apache.org/r/51027/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> note: check without filters depends on https://reviews.apache.org/r/51028 and 
> https://reviews.apache.org/r/52534
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

Re: Review Request 51027: Track allocation candidates to bound allocator.

[Benjamin Mahler]

> On Oct. 5, 2016, 1:02 a.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 1313-1318
> > 
> >
> > The intention of making allocate asynchronous and returning a Future is 
> > that the caller could tell when the asynchronous allocation completes and 
> > could set up a continuation based on this.
> > 
> > As it stands this function just returns Nothing always? Seems it should 
> > return to the caller once the caller's requested allocation completes.
> 
> Guangya Liu wrote:
> Here we want to enqueue only one `allocate(slaves)` request and then 
> update the `allocationCandidates` for other event trigger allocation requests 
> before the `allocate(slaves)` request was processed, after the 
> `allocate(slaves)` request was processed, another `allocate(slaves)` request 
> will be enqueued again, so seems we do not need to wait till the allocation 
> completes, but just check the allocation status for each `allocate(slaves)` 
> request, comments?

By "return to the caller once the allocation completes" I mean "return a Future 
that is satisfied once the allocation completes".


- Benjamin


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51027/#review151436
---


On Oct. 4, 2016, 11:31 p.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51027/
> ---
> 
> (Updated Oct. 4, 2016, 11:31 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Guangya Liu, James Peach, Klaus 
> Ma, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-3157
> https://issues.apache.org/jira/browse/MESOS-3157
> 
> 
> 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 
> 2c31471ee0f5d6836393bf87ff9ecfd8df835013 
>   src/master/allocator/mesos/hierarchical.cpp 
> c8f9492ee1b69e125a1e841116d22a578a9b524e 
> 
> Diff: https://reviews.apache.org/r/51027/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> note: check without filters depends on https://reviews.apache.org/r/51028 and 
> https://reviews.apache.org/r/52534
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

Re: Review Request 51027: Track allocation candidates to bound allocator.

[Guangya Liu]

> On 十月 5, 2016, 1:02 a.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 1313-1318
> > 
> >
> > The intention of making allocate asynchronous and returning a Future is 
> > that the caller could tell when the asynchronous allocation completes and 
> > could set up a continuation based on this.
> > 
> > As it stands this function just returns Nothing always? Seems it should 
> > return to the caller once the caller's requested allocation completes.

Here we want to enqueue only one `allocate(slaves)` request and then update the 
`allocationCandidates` for other event trigger allocation requests before the 
`allocate(slaves)` request was processed, after the `allocate(slaves)` request 
was processed, another `allocate(slaves)` request will be enqueued again, so 
seems we do not need to wait till the allocation completes, but just check the 
allocation status for each `allocate(slaves)` request, comments?


- Guangya


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51027/#review151436
---


On 十月 4, 2016, 11:31 p.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51027/
> ---
> 
> (Updated 十月 4, 2016, 11:31 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Guangya Liu, James Peach, Klaus 
> Ma, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-3157
> https://issues.apache.org/jira/browse/MESOS-3157
> 
> 
> 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 
> 2c31471ee0f5d6836393bf87ff9ecfd8df835013 
>   src/master/allocator/mesos/hierarchical.cpp 
> c8f9492ee1b69e125a1e841116d22a578a9b524e 
> 
> Diff: https://reviews.apache.org/r/51027/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> note: check without filters depends on https://reviews.apache.org/r/51028 and 
> https://reviews.apache.org/r/52534
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

Re: Review Request 51027: Track allocation candidates to bound allocator.

[Benjamin Mahler]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51027/#review151436
---




src/master/allocator/mesos/hierarchical.cpp (lines 1305 - 1310)


The intention of making allocate asynchronous and returning a Future is 
that the caller could tell when the asynchronous allocation completes and could 
set up a continuation based on this.

As it stands this function just returns Nothing always? Seems it should 
return to the caller once the caller's requested allocation completes.


- Benjamin Mahler


On Oct. 4, 2016, 11:31 p.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51027/
> ---
> 
> (Updated Oct. 4, 2016, 11:31 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Guangya Liu, James Peach, Klaus 
> Ma, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-3157
> https://issues.apache.org/jira/browse/MESOS-3157
> 
> 
> 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 
> 2c31471ee0f5d6836393bf87ff9ecfd8df835013 
>   src/master/allocator/mesos/hierarchical.cpp 
> c8f9492ee1b69e125a1e841116d22a578a9b524e 
> 
> Diff: https://reviews.apache.org/r/51027/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> note: check without filters depends on https://reviews.apache.org/r/51028 and 
> https://reviews.apache.org/r/52534
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

Re: Review Request 51027: Track allocation candidates to bound allocator.

[Jacob Janco]

> On Oct. 4, 2016, 9:47 p.m., Guangya Liu wrote:
> > Jacob, two other comments for this:
> > 
> > 1) when will you have the patch `SmallOfferFilter` ready for review?
> > 2) I think we may need another benchmark test which only do two operations: 
> > a) add multiple agents first 2) add multiple frameworks, this test can make 
> > sure we `batched` the `allocate()` request for all frameworks.

Added the fix for you: https://reviews.apache.org/r/52534


- Jacob


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51027/#review151408
---


On Oct. 4, 2016, 11:31 p.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51027/
> ---
> 
> (Updated Oct. 4, 2016, 11:31 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Guangya Liu, James Peach, Klaus 
> Ma, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-3157
> https://issues.apache.org/jira/browse/MESOS-3157
> 
> 
> 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 
> 2c31471ee0f5d6836393bf87ff9ecfd8df835013 
>   src/master/allocator/mesos/hierarchical.cpp 
> c8f9492ee1b69e125a1e841116d22a578a9b524e 
> 
> Diff: https://reviews.apache.org/r/51027/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> note: check without filters depends on https://reviews.apache.org/r/51028 and 
> https://reviews.apache.org/r/52534
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

Re: Review Request 51027: Track allocation candidates to bound allocator.

[Jacob Janco]

> On Oct. 1, 2016, 12:11 p.m., Guangya Liu wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 1311
> > 
> >
> > I think that here we should return `Nothing()` but not 
> > `allocation.get()`, as `allocation.get()` will wait till `allocation is 
> > ready`, but here we do not need to wait the till the `allocation become 
> > ready` but just update the `allocationCandidates` as soon as possible.

Since we're not doing anything with this future, I think it is safe to update 
allocation/allocationCandidates here and return Nothing()


- Jacob


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51027/#review151117
---


On Oct. 4, 2016, 11:31 p.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51027/
> ---
> 
> (Updated Oct. 4, 2016, 11:31 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Guangya Liu, James Peach, Klaus 
> Ma, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-3157
> https://issues.apache.org/jira/browse/MESOS-3157
> 
> 
> 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 
> 2c31471ee0f5d6836393bf87ff9ecfd8df835013 
>   src/master/allocator/mesos/hierarchical.cpp 
> c8f9492ee1b69e125a1e841116d22a578a9b524e 
> 
> Diff: https://reviews.apache.org/r/51027/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> note: check without filters depends on https://reviews.apache.org/r/51028 and 
> https://reviews.apache.org/r/52534
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

Re: Review Request 51027: Track allocation candidates to bound allocator.

[Jacob Janco]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51027/
---

(Updated Oct. 4, 2016, 11:31 p.m.)


Review request for mesos, Benjamin Mahler, Guangya Liu, James Peach, Klaus Ma, 
and Jiang Yan Xu.


Bugs: MESOS-3157
https://issues.apache.org/jira/browse/MESOS-3157


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 
2c31471ee0f5d6836393bf87ff9ecfd8df835013 
  src/master/allocator/mesos/hierarchical.cpp 
c8f9492ee1b69e125a1e841116d22a578a9b524e 

Diff: https://reviews.apache.org/r/51027/diff/


Testing (updated)
---

make check

note: check without filters depends on https://reviews.apache.org/r/51028 and 
https://reviews.apache.org/r/52534


Thanks,

Jacob Janco

Re: Review Request 51027: Track allocation candidates to bound allocator.

[Guangya Liu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51027/#review151408
---



Jacob, two other comments for this:

1) when will you have the patch `SmallOfferFilter` ready for review?
2) I think we may need another benchmark test which only do two operations: a) 
add multiple agents first 2) add multiple frameworks, this test can make sure 
we `batched` the `allocate()` request for all frameworks.


src/master/allocator/mesos/hierarchical.cpp (line 1298)


The `addFramework` will call `allocate()` and allocate `all agents 
resources` but not a `single agnent`, so here we should not mention 
`addFramework`, but only the following three:

1) addSlave
2) updateSlave
3) updateUnavailability?



src/master/allocator/mesos/hierarchical.cpp (line 1300)


s/slaveIds/`slaveIds`


- Guangya Liu


On 十月 4, 2016, 9:31 p.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51027/
> ---
> 
> (Updated 十月 4, 2016, 9:31 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Guangya Liu, James Peach, Klaus 
> Ma, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-3157
> https://issues.apache.org/jira/browse/MESOS-3157
> 
> 
> 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 
> 2c31471ee0f5d6836393bf87ff9ecfd8df835013 
>   src/master/allocator/mesos/hierarchical.cpp 
> c8f9492ee1b69e125a1e841116d22a578a9b524e 
> 
> Diff: https://reviews.apache.org/r/51027/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> note: check without filters depends on https://reviews.apache.org/r/51028
> note: SmallOfferFilter fix pending review -> will list Jira here
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

Re: Review Request 51027: Track allocation candidates to bound allocator.

[Jacob Janco]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51027/
---

(Updated Oct. 4, 2016, 9:31 p.m.)


Review request for mesos, Benjamin Mahler, Guangya Liu, James Peach, Klaus Ma, 
and Jiang Yan Xu.


Bugs: MESOS-3157
https://issues.apache.org/jira/browse/MESOS-3157


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 
2c31471ee0f5d6836393bf87ff9ecfd8df835013 
  src/master/allocator/mesos/hierarchical.cpp 
c8f9492ee1b69e125a1e841116d22a578a9b524e 

Diff: https://reviews.apache.org/r/51027/diff/


Testing
---

make check

note: check without filters depends on https://reviews.apache.org/r/51028
note: SmallOfferFilter fix pending review -> will list Jira here


Thanks,

Jacob Janco

Re: Review Request 51027: Track allocation candidates to bound allocator.

[Jacob Janco]

> On Sept. 28, 2016, 9:33 p.m., Guangya Liu wrote:
> > src/master/allocator/mesos/hierarchical.hpp, line 224
> > 
> >
> > ```
> > // If no run is queued we only update `allocationCandidates`
> > ```
> > 
> > I think here should be 
> > 
> > ```
> > We only update the `allocationCandidates` if there are pending 
> > allocation run.
> > ```

Changed to: 

// If a run is queued, we only update `allocationCandidates`.


- Jacob


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51027/#review150760
---


On Sept. 28, 2016, 8:21 p.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51027/
> ---
> 
> (Updated Sept. 28, 2016, 8:21 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Guangya Liu, James Peach, Klaus 
> Ma, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-3157
> https://issues.apache.org/jira/browse/MESOS-3157
> 
> 
> 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 
> 2c31471ee0f5d6836393bf87ff9ecfd8df835013 
>   src/master/allocator/mesos/hierarchical.cpp 
> 3f51f4194c1ba7c1e4f08c3dd623281ca5754d39 
> 
> Diff: https://reviews.apache.org/r/51027/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> note: check without filters depends on https://reviews.apache.org/r/51028
> note: SmallOfferFilter fix pending review -> will list Jira here
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

Re: Review Request 51027: Track allocation candidates to bound allocator.

[Guangya Liu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51027/#review151117
---




src/master/allocator/mesos/hierarchical.cpp (line 1303)


I think that here we should return `Nothing()` but not `allocation.get()`, 
as `allocation.get()` will wait till `allocation is ready`, but here we do not 
need to wait the till the `allocation become ready` but just update the 
`allocationCandidates` as soon as possible.


- Guangya Liu


On 九月 28, 2016, 8:21 p.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51027/
> ---
> 
> (Updated 九月 28, 2016, 8:21 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Guangya Liu, James Peach, Klaus 
> Ma, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-3157
> https://issues.apache.org/jira/browse/MESOS-3157
> 
> 
> 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 
> 2c31471ee0f5d6836393bf87ff9ecfd8df835013 
>   src/master/allocator/mesos/hierarchical.cpp 
> 3f51f4194c1ba7c1e4f08c3dd623281ca5754d39 
> 
> Diff: https://reviews.apache.org/r/51027/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> note: check without filters depends on https://reviews.apache.org/r/51028
> note: SmallOfferFilter fix pending review -> will list Jira here
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

Re: Review Request 51027: Track allocation candidates to bound allocator.

[Guangya Liu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51027/#review150760
---




src/master/allocator/mesos/hierarchical.hpp (line 224)


```
// If no run is queued we only update `allocationCandidates`
```

I think here should be 

```
We only update the `allocationCandidates` if there are pending allocation 
run.
```



src/master/allocator/mesos/hierarchical.cpp (lines 1292 - 1296)


Can we highlight that this part is mainly used to add `slaveIds` to the 
`allocationCandidates` because of some event triggered allocations such as 
`addSlave`, `updateSlave`, `updateAvailable`?



src/master/allocator/mesos/hierarchical.cpp (line 1349)


s/a small subset of slaves/allocation candidates for this cycle ?


- Guangya Liu


On 九月 28, 2016, 8:21 p.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51027/
> ---
> 
> (Updated 九月 28, 2016, 8:21 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Guangya Liu, James Peach, Klaus 
> Ma, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-3157
> https://issues.apache.org/jira/browse/MESOS-3157
> 
> 
> 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 
> 2c31471ee0f5d6836393bf87ff9ecfd8df835013 
>   src/master/allocator/mesos/hierarchical.cpp 
> 3f51f4194c1ba7c1e4f08c3dd623281ca5754d39 
> 
> Diff: https://reviews.apache.org/r/51027/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> note: check without filters depends on https://reviews.apache.org/r/51028
> note: SmallOfferFilter fix pending review -> will list Jira here
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

Re: Review Request 51027: Track allocation candidates to bound allocator.

[Jacob Janco]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51027/
---

(Updated Sept. 28, 2016, 8:21 p.m.)


Review request for mesos, Benjamin Mahler, Guangya Liu, James Peach, Klaus Ma, 
and Jiang Yan Xu.


Bugs: MESOS-3157
https://issues.apache.org/jira/browse/MESOS-3157


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 
2c31471ee0f5d6836393bf87ff9ecfd8df835013 
  src/master/allocator/mesos/hierarchical.cpp 
3f51f4194c1ba7c1e4f08c3dd623281ca5754d39 

Diff: https://reviews.apache.org/r/51027/diff/


Testing (updated)
---

make check

note: check without filters depends on https://reviews.apache.org/r/51028
note: SmallOfferFilter fix pending review -> will list Jira here


Thanks,

Jacob Janco

Re: Review Request 51027: Track allocation candidates to bound allocator.

[Guangya Liu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51027/#review150508
---



Looking to the test update.


src/master/allocator/mesos/hierarchical.hpp (lines 223 - 224)


fit to one line



src/master/allocator/mesos/hierarchical.hpp (lines 396 - 397)


Can you please update the comments a bit? I can see that the logic of add 
candidates for all events are actually centralized in `allocate(const 
hashset& slaveIds);` via `allocationCandidates = allocationCandidates 
| slaveIds;`, I cannot see the logic of add candidates triggered in `addSlave` 
or others.



src/master/allocator/mesos/hierarchical.cpp (lines 1810 - 1811)


I think it's not good to put it here, what about moving this to #1324?

This can make the allocate() more clear:

```
_allocate();

_deallocate();

allocationCandidates.clear();
```


- Guangya Liu


On 九月 26, 2016, 4:27 p.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51027/
> ---
> 
> (Updated 九月 26, 2016, 4:27 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Guangya Liu, James Peach, Klaus 
> Ma, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-3157
> https://issues.apache.org/jira/browse/MESOS-3157
> 
> 
> 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 
> 2c31471ee0f5d6836393bf87ff9ecfd8df835013 
>   src/master/allocator/mesos/hierarchical.cpp 
> 3f51f4194c1ba7c1e4f08c3dd623281ca5754d39 
> 
> Diff: https://reviews.apache.org/r/51027/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> note: check without filters depends on https://reviews.apache.org/r/51028
> 
> With new benchmark https://reviews.apache.org/r/49617: 
> Sample output without 51027:
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
> Using 1 agents and 3000 frameworks
> Added 3000 frameworks in 57251us
> Added 1 agents in 3.2134535333mins
> allocator settled after  1.6123603833mins
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
>  (290578 ms)
> 
> Sample output with 51027:
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
> Using 1 agents and 3000 frameworks
> Added 3000 frameworks in 39817us
> Added 1 agents in 3.2286054167mins
> allocator settled after  25.525654secs
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
>  (220137 ms)
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

Re: Review Request 51027: Track allocation candidates to bound allocator.

[Jacob Janco]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51027/
---

(Updated Sept. 26, 2016, 4:27 p.m.)


Review request for mesos, Benjamin Mahler, Guangya Liu, James Peach, Klaus Ma, 
and Jiang Yan Xu.


Changes
---

Cleaned up code with Yan's help/suggestions:
- overloaded `allocate()` dispatches `run()`
- an allocation `run()` synchronously runs `_allocate()` and `_deallocate()`


Bugs: MESOS-3157
https://issues.apache.org/jira/browse/MESOS-3157


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 
2c31471ee0f5d6836393bf87ff9ecfd8df835013 
  src/master/allocator/mesos/hierarchical.cpp 
3f51f4194c1ba7c1e4f08c3dd623281ca5754d39 

Diff: https://reviews.apache.org/r/51027/diff/


Testing
---

make check

note: check without filters depends on https://reviews.apache.org/r/51028

With new benchmark https://reviews.apache.org/r/49617: 
Sample output without 51027:
[ RUN  ] 
SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
Using 1 agents and 3000 frameworks
Added 3000 frameworks in 57251us
Added 1 agents in 3.2134535333mins
allocator settled after  1.6123603833mins
[   OK ] 
SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
 (290578 ms)

Sample output with 51027:
[ RUN  ] 
SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
Using 1 agents and 3000 frameworks
Added 3000 frameworks in 39817us
Added 1 agents in 3.2286054167mins
allocator settled after  25.525654secs
[   OK ] 
SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
 (220137 ms)


Thanks,

Jacob Janco

Re: Review Request 51027: Track allocation candidates to bound allocator.

[Jacob Janco]

> On Sept. 23, 2016, 2:40 a.m., Guangya Liu wrote:
> > It is really weired that the performance of 
> > `SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.DeclineOffers/7`
> >  does not improve much when calling `addSlave`, need check more for why 
> > `addSlave` was same? Without fix, the `addSlave` will call `allocate` for 
> > each agent, but with the fix, only one `allocate` will be called
> > 
> > ```
> > without fix:
> > [==] Running 1 test from 1 test case.
> > [--] Global test environment set-up.
> > [--] 1 test from 
> > SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test
> > [ RUN  ] 
> > SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.DeclineOffers/7
> > Using 1000 agents and 6000 frameworks
> > Added 6000 frameworks in 122268us
> > Added 1000 agents in 42.037104secs
> > 
> > With fix:
> > [==] Running 1 test from 1 test case.
> > [--] Global test environment set-up.
> > [--] 1 test from 
> > SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test
> > [ RUN  ] 
> > SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.DeclineOffers/7
> > Using 1000 agents and 6000 frameworks
> > Added 6000 frameworks in 116107us
> > Added 1000 agents in 41.615396secs
> > ```
> 
> Guangya Liu wrote:
> Jacob, I did more test with the code on Aug 23, at which I posted some 
> result in this RR, and found that the test result is different, I did 
> following to get Aug 23 code.
> 
> ```
> LiuGuangyas-MacBook-Pro:build gyliu$ git checkout 
> 2f78a440ef4201c5b11fb92c225694e84a60369c
> 
> LiuGuangyas-MacBook-Pro:build gyliu$ git log -1
> commit 2f78a440ef4201c5b11fb92c225694e84a60369c
> Author: Gilbert Song 
> Date:   Mon Aug 22 13:00:58 2016 -0700
> 
> Fixed potential flakiness in ROOT_RecoverOrphanedPersistentVolume.
> 
> Review: https://reviews.apache.org/r/51271/
> ```
> 
> The test result seems still same as now (without your patch and the code 
> is get from Aug 23):
> 
> ```
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.DeclineOffers/7
> Using 1000 agents and 6000 frameworks
> Added 6000 frameworks in 144272us
> Added 1000 agents in 43.107001secs
> ```
> 
> But anyway, I think that we need find out why the performance for 
> `addSlave` was not improved based on your patch.
> 
> Jacob Janco wrote:
> Yes agreed, per our Slack discussions, I'll look into this. Thanks for 
> posting the followup.
> 
> Benjamin Mahler wrote:
> `addSlave()` is asynchronous and we do not wait for all of the 
> `addSlave()` futures to complete, so any speedup in `addSlave()` will only 
> affect the next caller that waits for a result from the allocator.
> 
> Benjamin Mahler wrote:
> Ah I missed that we do a `Clock::settle()`, nevermind :)
> 
> Guangya Liu wrote:
> Some thinking for why `addSlave` does not improve much...
> 
> Without Jacob's patch, the logic woule be:
> 
> ```
> addSlave -> allocate the single slave
> addSlave -> allocate the single slave
> addSlave -> allocate the single slave
> ...
> addSlave -> allocate the single slave
> ```
> 
> With Jacob's patch, the logic would be:
> 
> ```
> addSlave
> addSlave
> addSlave
> ...
> addSlave - > allocate for **all** of the slaves
> ```
> 
> The time elapsed by `allocate a single slave N times` with `allocate N 
> slaves in one allocate` request should not different much, the only 
> difference is one is looping the event queue while another is looping in 
> allocator, that's why there are not enough performance change for this.
> 
> But this will impact a lot when adding frameworks or some other events in 
> allocator which will call `allocate(slaves)`, one proposal is we may need to 
> add some new benchmark test cases which do the following logic, the following 
> logic will trigger each `addframework` operation call `allocate(slaves)` 
> without Jacob's patch, but will only call `allocate(slaves)` one time with 
> Jacob's patch.
> 
> ```
> 1) Add slaves first
> 2) Add frameworks
> ```
> 
> We may get some performance improvement with above case.
> 
> Currently, all of the benchmark test are using 
> 
> ```
> 1) Add frameworks
> 2) Add agents
> ```
> 
> That's why not much performance improvement...
> 
> Jacob Janco wrote:
> This makes sense Guangya, I'm in the process of creating a minimal 
> benchmark adding a set of slaves then adding frameworks. I'll post here if 
> the results are interesting.
> 
> Guangya Liu wrote:
> Jacob, just FYI, 

Re: Review Request 51027: Track allocation candidates to bound allocator.

[Guangya Liu]

> On 九月 23, 2016, 2:40 a.m., Guangya Liu wrote:
> > It is really weired that the performance of 
> > `SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.DeclineOffers/7`
> >  does not improve much when calling `addSlave`, need check more for why 
> > `addSlave` was same? Without fix, the `addSlave` will call `allocate` for 
> > each agent, but with the fix, only one `allocate` will be called
> > 
> > ```
> > without fix:
> > [==] Running 1 test from 1 test case.
> > [--] Global test environment set-up.
> > [--] 1 test from 
> > SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test
> > [ RUN  ] 
> > SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.DeclineOffers/7
> > Using 1000 agents and 6000 frameworks
> > Added 6000 frameworks in 122268us
> > Added 1000 agents in 42.037104secs
> > 
> > With fix:
> > [==] Running 1 test from 1 test case.
> > [--] Global test environment set-up.
> > [--] 1 test from 
> > SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test
> > [ RUN  ] 
> > SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.DeclineOffers/7
> > Using 1000 agents and 6000 frameworks
> > Added 6000 frameworks in 116107us
> > Added 1000 agents in 41.615396secs
> > ```
> 
> Guangya Liu wrote:
> Jacob, I did more test with the code on Aug 23, at which I posted some 
> result in this RR, and found that the test result is different, I did 
> following to get Aug 23 code.
> 
> ```
> LiuGuangyas-MacBook-Pro:build gyliu$ git checkout 
> 2f78a440ef4201c5b11fb92c225694e84a60369c
> 
> LiuGuangyas-MacBook-Pro:build gyliu$ git log -1
> commit 2f78a440ef4201c5b11fb92c225694e84a60369c
> Author: Gilbert Song 
> Date:   Mon Aug 22 13:00:58 2016 -0700
> 
> Fixed potential flakiness in ROOT_RecoverOrphanedPersistentVolume.
> 
> Review: https://reviews.apache.org/r/51271/
> ```
> 
> The test result seems still same as now (without your patch and the code 
> is get from Aug 23):
> 
> ```
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.DeclineOffers/7
> Using 1000 agents and 6000 frameworks
> Added 6000 frameworks in 144272us
> Added 1000 agents in 43.107001secs
> ```
> 
> But anyway, I think that we need find out why the performance for 
> `addSlave` was not improved based on your patch.
> 
> Jacob Janco wrote:
> Yes agreed, per our Slack discussions, I'll look into this. Thanks for 
> posting the followup.
> 
> Benjamin Mahler wrote:
> `addSlave()` is asynchronous and we do not wait for all of the 
> `addSlave()` futures to complete, so any speedup in `addSlave()` will only 
> affect the next caller that waits for a result from the allocator.
> 
> Benjamin Mahler wrote:
> Ah I missed that we do a `Clock::settle()`, nevermind :)
> 
> Guangya Liu wrote:
> Some thinking for why `addSlave` does not improve much...
> 
> Without Jacob's patch, the logic woule be:
> 
> ```
> addSlave -> allocate the single slave
> addSlave -> allocate the single slave
> addSlave -> allocate the single slave
> ...
> addSlave -> allocate the single slave
> ```
> 
> With Jacob's patch, the logic would be:
> 
> ```
> addSlave
> addSlave
> addSlave
> ...
> addSlave - > allocate for **all** of the slaves
> ```
> 
> The time elapsed by `allocate a single slave N times` with `allocate N 
> slaves in one allocate` request should not different much, the only 
> difference is one is looping the event queue while another is looping in 
> allocator, that's why there are not enough performance change for this.
> 
> But this will impact a lot when adding frameworks or some other events in 
> allocator which will call `allocate(slaves)`, one proposal is we may need to 
> add some new benchmark test cases which do the following logic, the following 
> logic will trigger each `addframework` operation call `allocate(slaves)` 
> without Jacob's patch, but will only call `allocate(slaves)` one time with 
> Jacob's patch.
> 
> ```
> 1) Add slaves first
> 2) Add frameworks
> ```
> 
> We may get some performance improvement with above case.
> 
> Currently, all of the benchmark test are using 
> 
> ```
> 1) Add frameworks
> 2) Add agents
> ```
> 
> That's why not much performance improvement...
> 
> Jacob Janco wrote:
> This makes sense Guangya, I'm in the process of creating a minimal 
> benchmark adding a set of slaves then adding frameworks. I'll post here if 
> the results are interesting.

Jacob, just FYI, your fix does help a lot for the 

Re: Review Request 51027: Track allocation candidates to bound allocator.

[Guangya Liu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51027/#review150281
---



I think that you are still working on the test cases as I saw many test are 
failing


src/master/allocator/mesos/hierarchical.hpp (line 214)


```
Allocate resources just from the specifed agents.
```



src/master/allocator/mesos/hierarchical.hpp (line 217)


```
// Allocate resources from all agents.
```



src/master/allocator/mesos/hierarchical.cpp (lines 1291 - 1292)


1) Kill the `/` to the end
2) Why using union of `pendingAllocationAgentIds and slaveIds` here? Why 
not use `pendingAllocationAgentIds` directly? Can you please add more comments 
for the reason here?


- Guangya Liu


On 九月 23, 2016, 4:32 p.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51027/
> ---
> 
> (Updated 九月 23, 2016, 4:32 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Guangya Liu, James Peach, Klaus 
> Ma, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-3157
> https://issues.apache.org/jira/browse/MESOS-3157
> 
> 
> 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 
> 2c31471ee0f5d6836393bf87ff9ecfd8df835013 
>   src/master/allocator/mesos/hierarchical.cpp 
> 2d56bd011f2c87c67a02d0ae467a4a537d36867e 
> 
> Diff: https://reviews.apache.org/r/51027/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> note: check without filters depends on https://reviews.apache.org/r/51028
> 
> With new benchmark https://reviews.apache.org/r/49617: 
> Sample output without 51027:
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
> Using 1 agents and 3000 frameworks
> Added 3000 frameworks in 57251us
> Added 1 agents in 3.2134535333mins
> allocator settled after  1.6123603833mins
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
>  (290578 ms)
> 
> Sample output with 51027:
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
> Using 1 agents and 3000 frameworks
> Added 3000 frameworks in 39817us
> Added 1 agents in 3.2286054167mins
> allocator settled after  25.525654secs
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
>  (220137 ms)
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

Re: Review Request 51027: Track allocation candidates to bound allocator.

[Jacob Janco]

> On Sept. 23, 2016, 2:40 a.m., Guangya Liu wrote:
> > It is really weired that the performance of 
> > `SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.DeclineOffers/7`
> >  does not improve much when calling `addSlave`, need check more for why 
> > `addSlave` was same? Without fix, the `addSlave` will call `allocate` for 
> > each agent, but with the fix, only one `allocate` will be called
> > 
> > ```
> > without fix:
> > [==] Running 1 test from 1 test case.
> > [--] Global test environment set-up.
> > [--] 1 test from 
> > SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test
> > [ RUN  ] 
> > SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.DeclineOffers/7
> > Using 1000 agents and 6000 frameworks
> > Added 6000 frameworks in 122268us
> > Added 1000 agents in 42.037104secs
> > 
> > With fix:
> > [==] Running 1 test from 1 test case.
> > [--] Global test environment set-up.
> > [--] 1 test from 
> > SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test
> > [ RUN  ] 
> > SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.DeclineOffers/7
> > Using 1000 agents and 6000 frameworks
> > Added 6000 frameworks in 116107us
> > Added 1000 agents in 41.615396secs
> > ```
> 
> Guangya Liu wrote:
> Jacob, I did more test with the code on Aug 23, at which I posted some 
> result in this RR, and found that the test result is different, I did 
> following to get Aug 23 code.
> 
> ```
> LiuGuangyas-MacBook-Pro:build gyliu$ git checkout 
> 2f78a440ef4201c5b11fb92c225694e84a60369c
> 
> LiuGuangyas-MacBook-Pro:build gyliu$ git log -1
> commit 2f78a440ef4201c5b11fb92c225694e84a60369c
> Author: Gilbert Song 
> Date:   Mon Aug 22 13:00:58 2016 -0700
> 
> Fixed potential flakiness in ROOT_RecoverOrphanedPersistentVolume.
> 
> Review: https://reviews.apache.org/r/51271/
> ```
> 
> The test result seems still same as now (without your patch and the code 
> is get from Aug 23):
> 
> ```
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.DeclineOffers/7
> Using 1000 agents and 6000 frameworks
> Added 6000 frameworks in 144272us
> Added 1000 agents in 43.107001secs
> ```
> 
> But anyway, I think that we need find out why the performance for 
> `addSlave` was not improved based on your patch.
> 
> Jacob Janco wrote:
> Yes agreed, per our Slack discussions, I'll look into this. Thanks for 
> posting the followup.
> 
> Benjamin Mahler wrote:
> `addSlave()` is asynchronous and we do not wait for all of the 
> `addSlave()` futures to complete, so any speedup in `addSlave()` will only 
> affect the next caller that waits for a result from the allocator.
> 
> Benjamin Mahler wrote:
> Ah I missed that we do a `Clock::settle()`, nevermind :)
> 
> Guangya Liu wrote:
> Some thinking for why `addSlave` does not improve much...
> 
> Without Jacob's patch, the logic woule be:
> 
> ```
> addSlave -> allocate the single slave
> addSlave -> allocate the single slave
> addSlave -> allocate the single slave
> ...
> addSlave -> allocate the single slave
> ```
> 
> With Jacob's patch, the logic would be:
> 
> ```
> addSlave
> addSlave
> addSlave
> ...
> addSlave - > allocate for **all** of the slaves
> ```
> 
> The time elapsed by `allocate a single slave N times` with `allocate N 
> slaves in one allocate` request should not different much, the only 
> difference is one is looping the event queue while another is looping in 
> allocator, that's why there are not enough performance change for this.
> 
> But this will impact a lot when adding frameworks or some other events in 
> allocator which will call `allocate(slaves)`, one proposal is we may need to 
> add some new benchmark test cases which do the following logic, the following 
> logic will trigger each `addframework` operation call `allocate(slaves)` 
> without Jacob's patch, but will only call `allocate(slaves)` one time with 
> Jacob's patch.
> 
> ```
> 1) Add slaves first
> 2) Add frameworks
> ```
> 
> We may get some performance improvement with above case.
> 
> Currently, all of the benchmark test are using 
> 
> ```
> 1) Add frameworks
> 2) Add agents
> ```
> 
> That's why not much performance improvement...

This makes sense Guangya, I'm in the process of creating a minimal benchmark 
adding a set of slaves then adding frameworks. I'll post here if the results 
are interesting.


- Jacob


---
This is 

Re: Review Request 51027: Track allocation candidates to bound allocator.

[Guangya Liu]

> On 九月 23, 2016, 2:40 a.m., Guangya Liu wrote:
> > It is really weired that the performance of 
> > `SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.DeclineOffers/7`
> >  does not improve much when calling `addSlave`, need check more for why 
> > `addSlave` was same? Without fix, the `addSlave` will call `allocate` for 
> > each agent, but with the fix, only one `allocate` will be called
> > 
> > ```
> > without fix:
> > [==] Running 1 test from 1 test case.
> > [--] Global test environment set-up.
> > [--] 1 test from 
> > SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test
> > [ RUN  ] 
> > SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.DeclineOffers/7
> > Using 1000 agents and 6000 frameworks
> > Added 6000 frameworks in 122268us
> > Added 1000 agents in 42.037104secs
> > 
> > With fix:
> > [==] Running 1 test from 1 test case.
> > [--] Global test environment set-up.
> > [--] 1 test from 
> > SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test
> > [ RUN  ] 
> > SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.DeclineOffers/7
> > Using 1000 agents and 6000 frameworks
> > Added 6000 frameworks in 116107us
> > Added 1000 agents in 41.615396secs
> > ```
> 
> Guangya Liu wrote:
> Jacob, I did more test with the code on Aug 23, at which I posted some 
> result in this RR, and found that the test result is different, I did 
> following to get Aug 23 code.
> 
> ```
> LiuGuangyas-MacBook-Pro:build gyliu$ git checkout 
> 2f78a440ef4201c5b11fb92c225694e84a60369c
> 
> LiuGuangyas-MacBook-Pro:build gyliu$ git log -1
> commit 2f78a440ef4201c5b11fb92c225694e84a60369c
> Author: Gilbert Song 
> Date:   Mon Aug 22 13:00:58 2016 -0700
> 
> Fixed potential flakiness in ROOT_RecoverOrphanedPersistentVolume.
> 
> Review: https://reviews.apache.org/r/51271/
> ```
> 
> The test result seems still same as now (without your patch and the code 
> is get from Aug 23):
> 
> ```
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.DeclineOffers/7
> Using 1000 agents and 6000 frameworks
> Added 6000 frameworks in 144272us
> Added 1000 agents in 43.107001secs
> ```
> 
> But anyway, I think that we need find out why the performance for 
> `addSlave` was not improved based on your patch.
> 
> Jacob Janco wrote:
> Yes agreed, per our Slack discussions, I'll look into this. Thanks for 
> posting the followup.
> 
> Benjamin Mahler wrote:
> `addSlave()` is asynchronous and we do not wait for all of the 
> `addSlave()` futures to complete, so any speedup in `addSlave()` will only 
> affect the next caller that waits for a result from the allocator.
> 
> Benjamin Mahler wrote:
> Ah I missed that we do a `Clock::settle()`, nevermind :)

Some thinking for why `addSlave` does not improve much...

Without Jacob's patch, the logic woule be:

```
addSlave -> allocate the single slave
addSlave -> allocate the single slave
addSlave -> allocate the single slave
...
addSlave -> allocate the single slave
```

With Jacob's patch, the logic would be:

```
addSlave
addSlave
addSlave
...
addSlave - > allocate for **all** of the slaves
```

The time elapsed by `allocate a single slave N times` with `allocate N slaves 
in one allocate` request should not different much, the only difference is one 
is looping the event queue while another is looping in allocator, that's why 
there are not enough performance change for this.

But this will impact a lot when adding frameworks or some other events in 
allocator which will call `allocate(slaves)`, one proposal is we may need to 
add some new benchmark test cases which do the following logic, the following 
logic will trigger each `addframework` operation call `allocate(slaves)` 
without Jacob's patch, but will only call `allocate(slaves)` one time with 
Jacob's patch.

```
1) Add slaves first
2) Add frameworks
```

We may get some performance improvement with above case.

Currently, all of the benchmark test are using 

```
1) Add frameworks
2) Add agents
```

That's why not much performance improvement...


- Guangya


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51027/#review150123
---


On 九月 23, 2016, 4:32 p.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51027/
> ---
> 
> (Updated 九月 23, 

Re: Review Request 51027: Track allocation candidates to bound allocator.

[Benjamin Mahler]

> On Sept. 23, 2016, 2:40 a.m., Guangya Liu wrote:
> > It is really weired that the performance of 
> > `SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.DeclineOffers/7`
> >  does not improve much when calling `addSlave`, need check more for why 
> > `addSlave` was same? Without fix, the `addSlave` will call `allocate` for 
> > each agent, but with the fix, only one `allocate` will be called
> > 
> > ```
> > without fix:
> > [==] Running 1 test from 1 test case.
> > [--] Global test environment set-up.
> > [--] 1 test from 
> > SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test
> > [ RUN  ] 
> > SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.DeclineOffers/7
> > Using 1000 agents and 6000 frameworks
> > Added 6000 frameworks in 122268us
> > Added 1000 agents in 42.037104secs
> > 
> > With fix:
> > [==] Running 1 test from 1 test case.
> > [--] Global test environment set-up.
> > [--] 1 test from 
> > SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test
> > [ RUN  ] 
> > SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.DeclineOffers/7
> > Using 1000 agents and 6000 frameworks
> > Added 6000 frameworks in 116107us
> > Added 1000 agents in 41.615396secs
> > ```
> 
> Guangya Liu wrote:
> Jacob, I did more test with the code on Aug 23, at which I posted some 
> result in this RR, and found that the test result is different, I did 
> following to get Aug 23 code.
> 
> ```
> LiuGuangyas-MacBook-Pro:build gyliu$ git checkout 
> 2f78a440ef4201c5b11fb92c225694e84a60369c
> 
> LiuGuangyas-MacBook-Pro:build gyliu$ git log -1
> commit 2f78a440ef4201c5b11fb92c225694e84a60369c
> Author: Gilbert Song 
> Date:   Mon Aug 22 13:00:58 2016 -0700
> 
> Fixed potential flakiness in ROOT_RecoverOrphanedPersistentVolume.
> 
> Review: https://reviews.apache.org/r/51271/
> ```
> 
> The test result seems still same as now (without your patch and the code 
> is get from Aug 23):
> 
> ```
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.DeclineOffers/7
> Using 1000 agents and 6000 frameworks
> Added 6000 frameworks in 144272us
> Added 1000 agents in 43.107001secs
> ```
> 
> But anyway, I think that we need find out why the performance for 
> `addSlave` was not improved based on your patch.
> 
> Jacob Janco wrote:
> Yes agreed, per our Slack discussions, I'll look into this. Thanks for 
> posting the followup.
> 
> Benjamin Mahler wrote:
> `addSlave()` is asynchronous and we do not wait for all of the 
> `addSlave()` futures to complete, so any speedup in `addSlave()` will only 
> affect the next caller that waits for a result from the allocator.

Ah I missed that we do a `Clock::settle()`, nevermind :)


- Benjamin


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51027/#review150123
---


On Sept. 23, 2016, 4:32 p.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51027/
> ---
> 
> (Updated Sept. 23, 2016, 4:32 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Guangya Liu, James Peach, Klaus 
> Ma, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-3157
> https://issues.apache.org/jira/browse/MESOS-3157
> 
> 
> 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 
> 2c31471ee0f5d6836393bf87ff9ecfd8df835013 
>   src/master/allocator/mesos/hierarchical.cpp 
> 2d56bd011f2c87c67a02d0ae467a4a537d36867e 
> 
> Diff: https://reviews.apache.org/r/51027/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> note: check without filters depends on https://reviews.apache.org/r/51028
> 
> With new benchmark https://reviews.apache.org/r/49617: 
> Sample output without 51027:
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
> Using 1 agents and 3000 frameworks
> Added 3000 frameworks in 57251us
> Added 1 agents in 3.2134535333mins
> allocator settled after  1.6123603833mins
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
>  (290578 ms)
> 
> Sample output with 

Re: Review Request 51027: Track allocation candidates to bound allocator.

[Benjamin Mahler]

> On Sept. 23, 2016, 2:40 a.m., Guangya Liu wrote:
> > It is really weired that the performance of 
> > `SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.DeclineOffers/7`
> >  does not improve much when calling `addSlave`, need check more for why 
> > `addSlave` was same? Without fix, the `addSlave` will call `allocate` for 
> > each agent, but with the fix, only one `allocate` will be called
> > 
> > ```
> > without fix:
> > [==] Running 1 test from 1 test case.
> > [--] Global test environment set-up.
> > [--] 1 test from 
> > SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test
> > [ RUN  ] 
> > SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.DeclineOffers/7
> > Using 1000 agents and 6000 frameworks
> > Added 6000 frameworks in 122268us
> > Added 1000 agents in 42.037104secs
> > 
> > With fix:
> > [==] Running 1 test from 1 test case.
> > [--] Global test environment set-up.
> > [--] 1 test from 
> > SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test
> > [ RUN  ] 
> > SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.DeclineOffers/7
> > Using 1000 agents and 6000 frameworks
> > Added 6000 frameworks in 116107us
> > Added 1000 agents in 41.615396secs
> > ```
> 
> Guangya Liu wrote:
> Jacob, I did more test with the code on Aug 23, at which I posted some 
> result in this RR, and found that the test result is different, I did 
> following to get Aug 23 code.
> 
> ```
> LiuGuangyas-MacBook-Pro:build gyliu$ git checkout 
> 2f78a440ef4201c5b11fb92c225694e84a60369c
> 
> LiuGuangyas-MacBook-Pro:build gyliu$ git log -1
> commit 2f78a440ef4201c5b11fb92c225694e84a60369c
> Author: Gilbert Song 
> Date:   Mon Aug 22 13:00:58 2016 -0700
> 
> Fixed potential flakiness in ROOT_RecoverOrphanedPersistentVolume.
> 
> Review: https://reviews.apache.org/r/51271/
> ```
> 
> The test result seems still same as now (without your patch and the code 
> is get from Aug 23):
> 
> ```
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.DeclineOffers/7
> Using 1000 agents and 6000 frameworks
> Added 6000 frameworks in 144272us
> Added 1000 agents in 43.107001secs
> ```
> 
> But anyway, I think that we need find out why the performance for 
> `addSlave` was not improved based on your patch.
> 
> Jacob Janco wrote:
> Yes agreed, per our Slack discussions, I'll look into this. Thanks for 
> posting the followup.

`addSlave()` is asynchronous and we do not wait for all of the `addSlave()` 
futures to complete, so any speedup in `addSlave()` will only affect the next 
caller that waits for a result from the allocator.


- Benjamin


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51027/#review150123
---


On Sept. 23, 2016, 4:32 p.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51027/
> ---
> 
> (Updated Sept. 23, 2016, 4:32 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Guangya Liu, James Peach, Klaus 
> Ma, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-3157
> https://issues.apache.org/jira/browse/MESOS-3157
> 
> 
> 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 
> 2c31471ee0f5d6836393bf87ff9ecfd8df835013 
>   src/master/allocator/mesos/hierarchical.cpp 
> 2d56bd011f2c87c67a02d0ae467a4a537d36867e 
> 
> Diff: https://reviews.apache.org/r/51027/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> note: check without filters depends on https://reviews.apache.org/r/51028
> 
> With new benchmark https://reviews.apache.org/r/49617: 
> Sample output without 51027:
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
> Using 1 agents and 3000 frameworks
> Added 3000 frameworks in 57251us
> Added 1 agents in 3.2134535333mins
> allocator settled after  1.6123603833mins
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
>  (290578 ms)
> 
> Sample output with 51027:
> [ RUN  ] 
> 

Re: Review Request 51027: Track allocation candidates to bound allocator.

[Jacob Janco]

> On Sept. 23, 2016, 2:40 a.m., Guangya Liu wrote:
> > It is really weired that the performance of 
> > `SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.DeclineOffers/7`
> >  does not improve much when calling `addSlave`, need check more for why 
> > `addSlave` was same? Without fix, the `addSlave` will call `allocate` for 
> > each agent, but with the fix, only one `allocate` will be called
> > 
> > ```
> > without fix:
> > [==] Running 1 test from 1 test case.
> > [--] Global test environment set-up.
> > [--] 1 test from 
> > SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test
> > [ RUN  ] 
> > SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.DeclineOffers/7
> > Using 1000 agents and 6000 frameworks
> > Added 6000 frameworks in 122268us
> > Added 1000 agents in 42.037104secs
> > 
> > With fix:
> > [==] Running 1 test from 1 test case.
> > [--] Global test environment set-up.
> > [--] 1 test from 
> > SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test
> > [ RUN  ] 
> > SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.DeclineOffers/7
> > Using 1000 agents and 6000 frameworks
> > Added 6000 frameworks in 116107us
> > Added 1000 agents in 41.615396secs
> > ```
> 
> Guangya Liu wrote:
> Jacob, I did more test with the code on Aug 23, at which I posted some 
> result in this RR, and found that the test result is different, I did 
> following to get Aug 23 code.
> 
> ```
> LiuGuangyas-MacBook-Pro:build gyliu$ git checkout 
> 2f78a440ef4201c5b11fb92c225694e84a60369c
> 
> LiuGuangyas-MacBook-Pro:build gyliu$ git log -1
> commit 2f78a440ef4201c5b11fb92c225694e84a60369c
> Author: Gilbert Song 
> Date:   Mon Aug 22 13:00:58 2016 -0700
> 
> Fixed potential flakiness in ROOT_RecoverOrphanedPersistentVolume.
> 
> Review: https://reviews.apache.org/r/51271/
> ```
> 
> The test result seems still same as now (without your patch and the code 
> is get from Aug 23):
> 
> ```
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.DeclineOffers/7
> Using 1000 agents and 6000 frameworks
> Added 6000 frameworks in 144272us
> Added 1000 agents in 43.107001secs
> ```
> 
> But anyway, I think that we need find out why the performance for 
> `addSlave` was not improved based on your patch.

Yes agreed, per our Slack discussions, I'll look into this. Thanks for posting 
the followup.


- Jacob


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51027/#review150123
---


On Sept. 23, 2016, 4:32 p.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51027/
> ---
> 
> (Updated Sept. 23, 2016, 4:32 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Guangya Liu, James Peach, Klaus 
> Ma, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-3157
> https://issues.apache.org/jira/browse/MESOS-3157
> 
> 
> 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 
> 2c31471ee0f5d6836393bf87ff9ecfd8df835013 
>   src/master/allocator/mesos/hierarchical.cpp 
> 2d56bd011f2c87c67a02d0ae467a4a537d36867e 
> 
> Diff: https://reviews.apache.org/r/51027/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> note: check without filters depends on https://reviews.apache.org/r/51028
> 
> With new benchmark https://reviews.apache.org/r/49617: 
> Sample output without 51027:
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
> Using 1 agents and 3000 frameworks
> Added 3000 frameworks in 57251us
> Added 1 agents in 3.2134535333mins
> allocator settled after  1.6123603833mins
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
>  (290578 ms)
> 
> Sample output with 51027:
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
> Using 1 agents and 3000 frameworks
> Added 3000 frameworks in 39817us
> Added 1 agents in 3.2286054167mins
> allocator settled after  25.525654secs
> [   OK ] 
> 

Re: Review Request 51027: Track allocation candidates to bound allocator.

[Guangya Liu]

> On 九月 23, 2016, 2:40 a.m., Guangya Liu wrote:
> > It is really weired that the performance of 
> > `SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.DeclineOffers/7`
> >  does not improve much when calling `addSlave`, need check more for why 
> > `addSlave` was same? Without fix, the `addSlave` will call `allocate` for 
> > each agent, but with the fix, only one `allocate` will be called
> > 
> > ```
> > without fix:
> > [==] Running 1 test from 1 test case.
> > [--] Global test environment set-up.
> > [--] 1 test from 
> > SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test
> > [ RUN  ] 
> > SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.DeclineOffers/7
> > Using 1000 agents and 6000 frameworks
> > Added 6000 frameworks in 122268us
> > Added 1000 agents in 42.037104secs
> > 
> > With fix:
> > [==] Running 1 test from 1 test case.
> > [--] Global test environment set-up.
> > [--] 1 test from 
> > SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test
> > [ RUN  ] 
> > SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.DeclineOffers/7
> > Using 1000 agents and 6000 frameworks
> > Added 6000 frameworks in 116107us
> > Added 1000 agents in 41.615396secs
> > ```

Jacob, I did more test with the code on Aug 23, at which I posted some result 
in this RR, and found that the test result is different, I did following to get 
Aug 23 code.

```
LiuGuangyas-MacBook-Pro:build gyliu$ git checkout 
2f78a440ef4201c5b11fb92c225694e84a60369c

LiuGuangyas-MacBook-Pro:build gyliu$ git log -1
commit 2f78a440ef4201c5b11fb92c225694e84a60369c
Author: Gilbert Song 
Date:   Mon Aug 22 13:00:58 2016 -0700

Fixed potential flakiness in ROOT_RecoverOrphanedPersistentVolume.

Review: https://reviews.apache.org/r/51271/
```

The test result seems still same as now (without your patch and the code is get 
from Aug 23):

```
[==] Running 1 test from 1 test case.
[--] Global test environment set-up.
[--] 1 test from 
SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test
[ RUN  ] 
SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.DeclineOffers/7
Using 1000 agents and 6000 frameworks
Added 6000 frameworks in 144272us
Added 1000 agents in 43.107001secs
```

But anyway, I think that we need find out why the performance for `addSlave` 
was not improved based on your patch.


- Guangya


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51027/#review150123
---


On 九月 23, 2016, 4:32 p.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51027/
> ---
> 
> (Updated 九月 23, 2016, 4:32 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Guangya Liu, James Peach, Klaus 
> Ma, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-3157
> https://issues.apache.org/jira/browse/MESOS-3157
> 
> 
> 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 
> 2c31471ee0f5d6836393bf87ff9ecfd8df835013 
>   src/master/allocator/mesos/hierarchical.cpp 
> 2d56bd011f2c87c67a02d0ae467a4a537d36867e 
> 
> Diff: https://reviews.apache.org/r/51027/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> note: check without filters depends on https://reviews.apache.org/r/51028
> 
> With new benchmark https://reviews.apache.org/r/49617: 
> Sample output without 51027:
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
> Using 1 agents and 3000 frameworks
> Added 3000 frameworks in 57251us
> Added 1 agents in 3.2134535333mins
> allocator settled after  1.6123603833mins
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
>  (290578 ms)
> 
> Sample output with 51027:
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
> Using 1 agents and 3000 frameworks
> Added 3000 frameworks in 39817us
> Added 1 agents in 3.2286054167mins
> allocator settled after  25.525654secs
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
>  (220137 ms)
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

Re: Review Request 51027: Track allocation candidates to bound allocator.

[Jacob Janco]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51027/
---

(Updated Sept. 21, 2016, 5:41 p.m.)


Review request for mesos, Benjamin Mahler, Guangya Liu, James Peach, Klaus Ma, 
and Jiang Yan Xu.


Bugs: MESOS-3157
https://issues.apache.org/jira/browse/MESOS-3157


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.
- Batched allocations are handled synchronously.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.hpp 
2c31471ee0f5d6836393bf87ff9ecfd8df835013 
  src/master/allocator/mesos/hierarchical.cpp 
2d56bd011f2c87c67a02d0ae467a4a537d36867e 

Diff: https://reviews.apache.org/r/51027/diff/


Testing
---

make check

note: check without filters depends on https://reviews.apache.org/r/51028

With new benchmark https://reviews.apache.org/r/49617: 
Sample output without 51027:
[ RUN  ] 
SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
Using 1 agents and 3000 frameworks
Added 3000 frameworks in 57251us
Added 1 agents in 3.2134535333mins
allocator settled after  1.6123603833mins
[   OK ] 
SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
 (290578 ms)

Sample output with 51027:
[ RUN  ] 
SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
Using 1 agents and 3000 frameworks
Added 3000 frameworks in 39817us
Added 1 agents in 3.2286054167mins
allocator settled after  25.525654secs
[   OK ] 
SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
 (220137 ms)


Thanks,

Jacob Janco

Re: Review Request 51027: Track allocation candidates to bound allocator.

[Guangya Liu]

> On 九月 12, 2016, 8:46 p.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 273-274
> > 
> >
> > It seems a bit odd that the caller has to both touch allocation 
> > candidates and then call ensureAllocation.
> > 
> > A simpler way to think about this may be that `allocate` has changed 
> > from a synchronous function to an asynchronous one. I.e. the call-site here 
> > would be:
> > 
> > ```
> > void allocate(...); // becomes:
> > Future allocate(...);
> > 
> > // Call-site:
> > Future allocated = allocate(slave.keys());
> > 
> > // Actual allocation logic is a continuation now:
> > Nothing _allocate(...);
> > ```
> > 
> > (We probably do not need the Future just yet since callers don't need 
> > to set up continuations for now, however we might as well add it to express 
> > the asynchronous nature of the function.)
> > 
> > This also avoids the need for the callers to touch the data structure 
> > correctly, which seems error-prone. They just call `allocate` and it deals 
> > with adding the SlaveIDs that need allocations performed.

The reason that `the caller touch both allocation candidates and then call 
ensureAllocation` is because the caller need to handle different allocation 
event.

1) For some allocation event, we need to use `allocationCandidates = 
slaves.keys();` such as `addFramework` etc, as we want to get all free 
resources from the resource pool and allocate to the new framework.
2) For some allocation event, such as `addSlave`, there is no need to use 
`slaves.keys()` but only adding the new added agent to the 
`allocationCandidates` is good enough, this can reduce the time of the 
allocation cycle: Allocating only one agent is fast than allocating all agent 
in an allocation cycle if there are bunch of agents.
3) The `allocationCandidates` needs to be cleaned after each allocation cycle.


- Guangya


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51027/#review148579
---


On 九月 3, 2016, 6:05 a.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51027/
> ---
> 
> (Updated 九月 3, 2016, 6:05 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Guangya Liu, James Peach, Klaus 
> Ma, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-3157
> https://issues.apache.org/jira/browse/MESOS-3157
> 
> 
> 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.
> - Batched allocations are handled synchronously.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> dd07ed221d2c1755d2478369641ffdc46ecc4471 
>   src/master/allocator/mesos/hierarchical.cpp 
> 9e5db2196c6a541dc1208ba8b9f13ef9a518bcc4 
> 
> Diff: https://reviews.apache.org/r/51027/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> note: check without filters depends on https://reviews.apache.org/r/51028
> 
> With new benchmark https://reviews.apache.org/r/49617: 
> Sample output without 51027:
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
> Using 1 agents and 3000 frameworks
> Added 3000 frameworks in 57251us
> Added 1 agents in 3.2134535333mins
> allocator settled after  1.6123603833mins
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
>  (290578 ms)
> 
> Sample output with 51027:
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
> Using 1 agents and 3000 frameworks
> Added 3000 frameworks in 39817us
> Added 1 agents in 3.2286054167mins
> allocator settled after  25.525654secs
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
>  (220137 ms)
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

Re: Review Request 51027: Track allocation candidates to bound allocator.

[Benjamin Mahler]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51027/#review148579
---




src/master/allocator/mesos/hierarchical.hpp (lines 227 - 228)


Hm.. I'm having a hard time understanding what a unique dispatch is, or if 
it's a useful concept.

It seems to me that we could instead provide a generic operation batching 
abtraction that is provided as a library in libprocess (the registrar and the 
allocator could both use such an abstraction). This seems to be a clearer 
approach to me than providing some kind of alternate dispatch semantics 
(composition (build on top) vs inheritance (different flavors of dispatching)).



src/master/allocator/mesos/hierarchical.cpp (lines 272 - 273)


It seems a bit odd that the caller has to both touch allocation candidates 
and then call ensureAllocation.

A simpler way to think about this may be that `allocate` has changed from a 
synchronous function to an asynchronous one. I.e. the call-site here would be:

```
void allocate(...); // becomes:
Future allocate(...);

// Call-site:
Future allocated = allocate(slave.keys());

// Actual allocation logic is a continuation now:
Nothing _allocate(...);
```

(We probably do not need the Future just yet since callers don't need to 
set up continuations for now, however we might as well add it to express the 
asynchronous nature of the function.)

This also avoids the need for the callers to touch the data structure 
correctly, which seems error-prone. They just call `allocate` and it deals with 
adding the SlaveIDs that need allocations performed.


- Benjamin Mahler


On Sept. 3, 2016, 6:05 a.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51027/
> ---
> 
> (Updated Sept. 3, 2016, 6:05 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Guangya Liu, James Peach, Klaus 
> Ma, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-3157
> https://issues.apache.org/jira/browse/MESOS-3157
> 
> 
> 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.
> - Batched allocations are handled synchronously.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> dd07ed221d2c1755d2478369641ffdc46ecc4471 
>   src/master/allocator/mesos/hierarchical.cpp 
> 9e5db2196c6a541dc1208ba8b9f13ef9a518bcc4 
> 
> Diff: https://reviews.apache.org/r/51027/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> note: check without filters depends on https://reviews.apache.org/r/51028
> 
> With new benchmark https://reviews.apache.org/r/49617: 
> Sample output without 51027:
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
> Using 1 agents and 3000 frameworks
> Added 3000 frameworks in 57251us
> Added 1 agents in 3.2134535333mins
> allocator settled after  1.6123603833mins
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
>  (290578 ms)
> 
> Sample output with 51027:
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
> Using 1 agents and 3000 frameworks
> Added 3000 frameworks in 39817us
> Added 1 agents in 3.2286054167mins
> allocator settled after  25.525654secs
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
>  (220137 ms)
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

Re: Review Request 51027: Track allocation candidates to bound allocator.

[Jacob Janco]

> On Sept. 3, 2016, 9:21 a.m., Guangya Liu wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 484-485
> > 
> >
> > I did some test with benchmark test of 
> > `SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.AddAndUpdateSlave/0`
> >  and it failed as following:
> > 
> > ```
> > ../../src/tests/hierarchical_allocator_tests.cpp:3459: Failure
> > Value of: offerCallbacks.load()
> >   Actual: 5
> > Expected: slaveCount
> > Which is: 1000
> > [  FAILED  ] 
> > SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.AddAndUpdateSlave/0,
> >  where GetParam() = (1000, 1) (497 ms)
> > [--] 1 test from 
> > SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test (498 ms total)
> > 
> > [--] Global test environment tear-down
> > [==] 1 test from 1 test case ran. (512 ms total)
> > [  PASSED  ] 0 tests.
> > [  FAILED  ] 1 test, listed below:
> > [  FAILED  ] 
> > SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.AddAndUpdateSlave/0,
> >  where GetParam() = (1000, 1)
> > 
> >  1 FAILED TEST
> > ```
> > 
> > The reason is that we cannot make sure all of the `_allocate()` 
> > finished after `addSlave` finished.
> > 
> > Shall we do a `while` loop in the benchmark to wait till all 
> > allocations are got?
> 
> Guangya Liu wrote:
> Perhaps we can simply remove the `ASSERT_EQ` for offer and agent count in 
> `HierarchicalAllocator_BENCHMARK_Test.AddAndUpdateSlave` as this patch will 
> not trigger `allocate()` synchronously, so we cannot make sure the agent 
> count is always same as offer count.
> 
> Jacob Janco wrote:
> Good catch, I'll run the benchmarks and update 
> https://reviews.apache.org/r/51028/
> 
> Jacob Janco wrote:
> I updated that review if you could take a look. Thanks!
> 
> Guangya Liu wrote:
> I did not see the update, seems you did not publish your update? :-)
> 
> Jacob Janco wrote:
> Spoke too soon, I need to fix one of the tests before posting another 
> review.

Review posted!


- Jacob


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51027/#review147722
---


On Sept. 3, 2016, 6:05 a.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51027/
> ---
> 
> (Updated Sept. 3, 2016, 6:05 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Guangya Liu, James Peach, Klaus 
> Ma, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-3157
> https://issues.apache.org/jira/browse/MESOS-3157
> 
> 
> 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.
> - Batched allocations are handled synchronously.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> dd07ed221d2c1755d2478369641ffdc46ecc4471 
>   src/master/allocator/mesos/hierarchical.cpp 
> 9e5db2196c6a541dc1208ba8b9f13ef9a518bcc4 
> 
> Diff: https://reviews.apache.org/r/51027/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> note: check without filters depends on https://reviews.apache.org/r/51028
> 
> With new benchmark https://reviews.apache.org/r/49617: 
> Sample output without 51027:
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
> Using 1 agents and 3000 frameworks
> Added 3000 frameworks in 57251us
> Added 1 agents in 3.2134535333mins
> allocator settled after  1.6123603833mins
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
>  (290578 ms)
> 
> Sample output with 51027:
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
> Using 1 agents and 3000 frameworks
> Added 3000 frameworks in 39817us
> Added 1 agents in 3.2286054167mins
> allocator settled after  25.525654secs
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
>  (220137 ms)
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

Re: Review Request 51027: Track allocation candidates to bound allocator.

[Jacob Janco]

> On Sept. 3, 2016, 9:21 a.m., Guangya Liu wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 484-485
> > 
> >
> > I did some test with benchmark test of 
> > `SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.AddAndUpdateSlave/0`
> >  and it failed as following:
> > 
> > ```
> > ../../src/tests/hierarchical_allocator_tests.cpp:3459: Failure
> > Value of: offerCallbacks.load()
> >   Actual: 5
> > Expected: slaveCount
> > Which is: 1000
> > [  FAILED  ] 
> > SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.AddAndUpdateSlave/0,
> >  where GetParam() = (1000, 1) (497 ms)
> > [--] 1 test from 
> > SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test (498 ms total)
> > 
> > [--] Global test environment tear-down
> > [==] 1 test from 1 test case ran. (512 ms total)
> > [  PASSED  ] 0 tests.
> > [  FAILED  ] 1 test, listed below:
> > [  FAILED  ] 
> > SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.AddAndUpdateSlave/0,
> >  where GetParam() = (1000, 1)
> > 
> >  1 FAILED TEST
> > ```
> > 
> > The reason is that we cannot make sure all of the `_allocate()` 
> > finished after `addSlave` finished.
> > 
> > Shall we do a `while` loop in the benchmark to wait till all 
> > allocations are got?
> 
> Guangya Liu wrote:
> Perhaps we can simply remove the `ASSERT_EQ` for offer and agent count in 
> `HierarchicalAllocator_BENCHMARK_Test.AddAndUpdateSlave` as this patch will 
> not trigger `allocate()` synchronously, so we cannot make sure the agent 
> count is always same as offer count.
> 
> Jacob Janco wrote:
> Good catch, I'll run the benchmarks and update 
> https://reviews.apache.org/r/51028/
> 
> Jacob Janco wrote:
> I updated that review if you could take a look. Thanks!
> 
> Guangya Liu wrote:
> I did not see the update, seems you did not publish your update? :-)

Spoke too soon, I need to fix one of the tests before posting another review.


- Jacob


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51027/#review147722
---


On Sept. 3, 2016, 6:05 a.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51027/
> ---
> 
> (Updated Sept. 3, 2016, 6:05 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Guangya Liu, James Peach, Klaus 
> Ma, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-3157
> https://issues.apache.org/jira/browse/MESOS-3157
> 
> 
> 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.
> - Batched allocations are handled synchronously.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> dd07ed221d2c1755d2478369641ffdc46ecc4471 
>   src/master/allocator/mesos/hierarchical.cpp 
> 9e5db2196c6a541dc1208ba8b9f13ef9a518bcc4 
> 
> Diff: https://reviews.apache.org/r/51027/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> note: check without filters depends on https://reviews.apache.org/r/51028
> 
> With new benchmark https://reviews.apache.org/r/49617: 
> Sample output without 51027:
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
> Using 1 agents and 3000 frameworks
> Added 3000 frameworks in 57251us
> Added 1 agents in 3.2134535333mins
> allocator settled after  1.6123603833mins
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
>  (290578 ms)
> 
> Sample output with 51027:
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
> Using 1 agents and 3000 frameworks
> Added 3000 frameworks in 39817us
> Added 1 agents in 3.2286054167mins
> allocator settled after  25.525654secs
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
>  (220137 ms)
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

Re: Review Request 51027: Track allocation candidates to bound allocator.

[Guangya Liu]

> On 九月 3, 2016, 9:21 a.m., Guangya Liu wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 484-485
> > 
> >
> > I did some test with benchmark test of 
> > `SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.AddAndUpdateSlave/0`
> >  and it failed as following:
> > 
> > ```
> > ../../src/tests/hierarchical_allocator_tests.cpp:3459: Failure
> > Value of: offerCallbacks.load()
> >   Actual: 5
> > Expected: slaveCount
> > Which is: 1000
> > [  FAILED  ] 
> > SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.AddAndUpdateSlave/0,
> >  where GetParam() = (1000, 1) (497 ms)
> > [--] 1 test from 
> > SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test (498 ms total)
> > 
> > [--] Global test environment tear-down
> > [==] 1 test from 1 test case ran. (512 ms total)
> > [  PASSED  ] 0 tests.
> > [  FAILED  ] 1 test, listed below:
> > [  FAILED  ] 
> > SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.AddAndUpdateSlave/0,
> >  where GetParam() = (1000, 1)
> > 
> >  1 FAILED TEST
> > ```
> > 
> > The reason is that we cannot make sure all of the `_allocate()` 
> > finished after `addSlave` finished.
> > 
> > Shall we do a `while` loop in the benchmark to wait till all 
> > allocations are got?
> 
> Guangya Liu wrote:
> Perhaps we can simply remove the `ASSERT_EQ` for offer and agent count in 
> `HierarchicalAllocator_BENCHMARK_Test.AddAndUpdateSlave` as this patch will 
> not trigger `allocate()` synchronously, so we cannot make sure the agent 
> count is always same as offer count.
> 
> Jacob Janco wrote:
> Good catch, I'll run the benchmarks and update 
> https://reviews.apache.org/r/51028/
> 
> Jacob Janco wrote:
> I updated that review if you could take a look. Thanks!

I did not see the update, seems you did not publish your update? :-)


- Guangya


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51027/#review147722
---


On 九月 3, 2016, 6:05 a.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51027/
> ---
> 
> (Updated 九月 3, 2016, 6:05 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Guangya Liu, James Peach, Klaus 
> Ma, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-3157
> https://issues.apache.org/jira/browse/MESOS-3157
> 
> 
> 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.
> - Batched allocations are handled synchronously.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> dd07ed221d2c1755d2478369641ffdc46ecc4471 
>   src/master/allocator/mesos/hierarchical.cpp 
> 9e5db2196c6a541dc1208ba8b9f13ef9a518bcc4 
> 
> Diff: https://reviews.apache.org/r/51027/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> note: check without filters depends on https://reviews.apache.org/r/51028
> 
> With new benchmark https://reviews.apache.org/r/49617: 
> Sample output without 51027:
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
> Using 1 agents and 3000 frameworks
> Added 3000 frameworks in 57251us
> Added 1 agents in 3.2134535333mins
> allocator settled after  1.6123603833mins
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
>  (290578 ms)
> 
> Sample output with 51027:
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
> Using 1 agents and 3000 frameworks
> Added 3000 frameworks in 39817us
> Added 1 agents in 3.2286054167mins
> allocator settled after  25.525654secs
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
>  (220137 ms)
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

Re: Review Request 51027: Track allocation candidates to bound allocator.

[Jacob Janco]

> On Sept. 3, 2016, 9:21 a.m., Guangya Liu wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 484-485
> > 
> >
> > I did some test with benchmark test of 
> > `SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.AddAndUpdateSlave/0`
> >  and it failed as following:
> > 
> > ```
> > ../../src/tests/hierarchical_allocator_tests.cpp:3459: Failure
> > Value of: offerCallbacks.load()
> >   Actual: 5
> > Expected: slaveCount
> > Which is: 1000
> > [  FAILED  ] 
> > SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.AddAndUpdateSlave/0,
> >  where GetParam() = (1000, 1) (497 ms)
> > [--] 1 test from 
> > SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test (498 ms total)
> > 
> > [--] Global test environment tear-down
> > [==] 1 test from 1 test case ran. (512 ms total)
> > [  PASSED  ] 0 tests.
> > [  FAILED  ] 1 test, listed below:
> > [  FAILED  ] 
> > SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.AddAndUpdateSlave/0,
> >  where GetParam() = (1000, 1)
> > 
> >  1 FAILED TEST
> > ```
> > 
> > The reason is that we cannot make sure all of the `_allocate()` 
> > finished after `addSlave` finished.
> > 
> > Shall we do a `while` loop in the benchmark to wait till all 
> > allocations are got?
> 
> Guangya Liu wrote:
> Perhaps we can simply remove the `ASSERT_EQ` for offer and agent count in 
> `HierarchicalAllocator_BENCHMARK_Test.AddAndUpdateSlave` as this patch will 
> not trigger `allocate()` synchronously, so we cannot make sure the agent 
> count is always same as offer count.
> 
> Jacob Janco wrote:
> Good catch, I'll run the benchmarks and update 
> https://reviews.apache.org/r/51028/

I updated that review if you could take a look. Thanks!


- Jacob


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51027/#review147722
---


On Sept. 3, 2016, 6:05 a.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51027/
> ---
> 
> (Updated Sept. 3, 2016, 6:05 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Guangya Liu, James Peach, Klaus 
> Ma, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-3157
> https://issues.apache.org/jira/browse/MESOS-3157
> 
> 
> 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.
> - Batched allocations are handled synchronously.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> dd07ed221d2c1755d2478369641ffdc46ecc4471 
>   src/master/allocator/mesos/hierarchical.cpp 
> 9e5db2196c6a541dc1208ba8b9f13ef9a518bcc4 
> 
> Diff: https://reviews.apache.org/r/51027/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> note: check without filters depends on https://reviews.apache.org/r/51028
> 
> With new benchmark https://reviews.apache.org/r/49617: 
> Sample output without 51027:
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
> Using 1 agents and 3000 frameworks
> Added 3000 frameworks in 57251us
> Added 1 agents in 3.2134535333mins
> allocator settled after  1.6123603833mins
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
>  (290578 ms)
> 
> Sample output with 51027:
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
> Using 1 agents and 3000 frameworks
> Added 3000 frameworks in 39817us
> Added 1 agents in 3.2286054167mins
> allocator settled after  25.525654secs
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
>  (220137 ms)
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

Re: Review Request 51027: Track allocation candidates to bound allocator.

[Jacob Janco]

> On Sept. 3, 2016, 9:21 a.m., Guangya Liu wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 484-485
> > 
> >
> > I did some test with benchmark test of 
> > `SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.AddAndUpdateSlave/0`
> >  and it failed as following:
> > 
> > ```
> > ../../src/tests/hierarchical_allocator_tests.cpp:3459: Failure
> > Value of: offerCallbacks.load()
> >   Actual: 5
> > Expected: slaveCount
> > Which is: 1000
> > [  FAILED  ] 
> > SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.AddAndUpdateSlave/0,
> >  where GetParam() = (1000, 1) (497 ms)
> > [--] 1 test from 
> > SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test (498 ms total)
> > 
> > [--] Global test environment tear-down
> > [==] 1 test from 1 test case ran. (512 ms total)
> > [  PASSED  ] 0 tests.
> > [  FAILED  ] 1 test, listed below:
> > [  FAILED  ] 
> > SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.AddAndUpdateSlave/0,
> >  where GetParam() = (1000, 1)
> > 
> >  1 FAILED TEST
> > ```
> > 
> > The reason is that we cannot make sure all of the `_allocate()` 
> > finished after `addSlave` finished.
> > 
> > Shall we do a `while` loop in the benchmark to wait till all 
> > allocations are got?
> 
> Guangya Liu wrote:
> Perhaps we can simply remove the `ASSERT_EQ` for offer and agent count in 
> `HierarchicalAllocator_BENCHMARK_Test.AddAndUpdateSlave` as this patch will 
> not trigger `allocate()` synchronously, so we cannot make sure the agent 
> count is always same as offer count.

Good catch, I'll run the benchmarks and update 
https://reviews.apache.org/r/51028/


- Jacob


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51027/#review147722
---


On Sept. 3, 2016, 6:05 a.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51027/
> ---
> 
> (Updated Sept. 3, 2016, 6:05 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Guangya Liu, James Peach, Klaus 
> Ma, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-3157
> https://issues.apache.org/jira/browse/MESOS-3157
> 
> 
> 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.
> - Batched allocations are handled synchronously.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> dd07ed221d2c1755d2478369641ffdc46ecc4471 
>   src/master/allocator/mesos/hierarchical.cpp 
> 9e5db2196c6a541dc1208ba8b9f13ef9a518bcc4 
> 
> Diff: https://reviews.apache.org/r/51027/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> note: check without filters depends on https://reviews.apache.org/r/51028
> 
> With new benchmark https://reviews.apache.org/r/49617: 
> Sample output without 51027:
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
> Using 1 agents and 3000 frameworks
> Added 3000 frameworks in 57251us
> Added 1 agents in 3.2134535333mins
> allocator settled after  1.6123603833mins
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
>  (290578 ms)
> 
> Sample output with 51027:
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
> Using 1 agents and 3000 frameworks
> Added 3000 frameworks in 39817us
> Added 1 agents in 3.2286054167mins
> allocator settled after  25.525654secs
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
>  (220137 ms)
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

Re: Review Request 51027: Track allocation candidates to bound allocator.

[Jacob Janco]

> On Sept. 3, 2016, 6:38 a.m., Guangya Liu wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 1226-1229
> > 
> >
> > Just nit, I think that we should separate the `_allocate()` and 
> > `_deallocate()` to use different time elapse. But as the original logic is 
> > also putting the `_deallocate()` logic in `_allocate()`, so I think that it 
> > is OK to keep such logic but may need an update if we want to make the time 
> > more accurate for diffrent actions: `_allocate()` and `_deallocate()`.
> > 
> > ```
> > stopwatch.start();
> > 
> > metrics.allocation_run.start();
> > 
> > _allocate();
> > 
> > metrics.allocation_run.stop();
> > 
> > stopwatch.stop();
> > 
> > VLOG(1) << "Performed allocation for " << allocationCandidates.size()
> > << " agents in " << stopwatch.elapsed();
> > 
> > stopwatch.start();
> > 
> > _deallocate();
> > 
> > stopwatch.stop();
> > 
> > VLOG(1) << "Performed deallocation for " << allocationCandidates.size()
> > << " agents in " << stopwatch.elapsed();
> > ```

Yea, I think preserving the original behavior was the idea here so the metrics 
reflect that.


- Jacob


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51027/#review147718
---


On Sept. 3, 2016, 6:05 a.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51027/
> ---
> 
> (Updated Sept. 3, 2016, 6:05 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Guangya Liu, James Peach, Klaus 
> Ma, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-3157
> https://issues.apache.org/jira/browse/MESOS-3157
> 
> 
> 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.
> - Batched allocations are handled synchronously.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> dd07ed221d2c1755d2478369641ffdc46ecc4471 
>   src/master/allocator/mesos/hierarchical.cpp 
> 9e5db2196c6a541dc1208ba8b9f13ef9a518bcc4 
> 
> Diff: https://reviews.apache.org/r/51027/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> note: check without filters depends on https://reviews.apache.org/r/51028
> 
> With new benchmark https://reviews.apache.org/r/49617: 
> Sample output without 51027:
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
> Using 1 agents and 3000 frameworks
> Added 3000 frameworks in 57251us
> Added 1 agents in 3.2134535333mins
> allocator settled after  1.6123603833mins
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
>  (290578 ms)
> 
> Sample output with 51027:
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
> Using 1 agents and 3000 frameworks
> Added 3000 frameworks in 39817us
> Added 1 agents in 3.2286054167mins
> allocator settled after  25.525654secs
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
>  (220137 ms)
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

Re: Review Request 51027: Track allocation candidates to bound allocator.

[Guangya Liu]

> On 九月 3, 2016, 9:21 a.m., Guangya Liu wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 484-485
> > 
> >
> > I did some test with benchmark test of 
> > `SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.AddAndUpdateSlave/0`
> >  and it failed as following:
> > 
> > ```
> > ../../src/tests/hierarchical_allocator_tests.cpp:3459: Failure
> > Value of: offerCallbacks.load()
> >   Actual: 5
> > Expected: slaveCount
> > Which is: 1000
> > [  FAILED  ] 
> > SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.AddAndUpdateSlave/0,
> >  where GetParam() = (1000, 1) (497 ms)
> > [--] 1 test from 
> > SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test (498 ms total)
> > 
> > [--] Global test environment tear-down
> > [==] 1 test from 1 test case ran. (512 ms total)
> > [  PASSED  ] 0 tests.
> > [  FAILED  ] 1 test, listed below:
> > [  FAILED  ] 
> > SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.AddAndUpdateSlave/0,
> >  where GetParam() = (1000, 1)
> > 
> >  1 FAILED TEST
> > ```
> > 
> > The reason is that we cannot make sure all of the `_allocate()` 
> > finished after `addSlave` finished.
> > 
> > Shall we do a `while` loop in the benchmark to wait till all 
> > allocations are got?

Perhaps we can simply remove the `ASSERT_EQ` for offer and agent count in 
`HierarchicalAllocator_BENCHMARK_Test.AddAndUpdateSlave` as this patch will not 
trigger `allocate()` synchronously, so we cannot make sure the agent count is 
always same as offer count.


- Guangya


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51027/#review147722
---


On 九月 3, 2016, 6:05 a.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51027/
> ---
> 
> (Updated 九月 3, 2016, 6:05 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Guangya Liu, James Peach, Klaus 
> Ma, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-3157
> https://issues.apache.org/jira/browse/MESOS-3157
> 
> 
> 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.
> - Batched allocations are handled synchronously.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> dd07ed221d2c1755d2478369641ffdc46ecc4471 
>   src/master/allocator/mesos/hierarchical.cpp 
> 9e5db2196c6a541dc1208ba8b9f13ef9a518bcc4 
> 
> Diff: https://reviews.apache.org/r/51027/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> note: check without filters depends on https://reviews.apache.org/r/51028
> 
> With new benchmark https://reviews.apache.org/r/49617: 
> Sample output without 51027:
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
> Using 1 agents and 3000 frameworks
> Added 3000 frameworks in 57251us
> Added 1 agents in 3.2134535333mins
> allocator settled after  1.6123603833mins
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
>  (290578 ms)
> 
> Sample output with 51027:
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
> Using 1 agents and 3000 frameworks
> Added 3000 frameworks in 39817us
> Added 1 agents in 3.2286054167mins
> allocator settled after  25.525654secs
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
>  (220137 ms)
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

Re: Review Request 51027: Track allocation candidates to bound allocator.

[Guangya Liu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51027/#review147722
---




src/master/allocator/mesos/hierarchical.cpp (lines 483 - 484)


I did some test with benchmark test of 
`SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.AddAndUpdateSlave/0`
 and it failed as following:

```
../../src/tests/hierarchical_allocator_tests.cpp:3459: Failure
Value of: offerCallbacks.load()
  Actual: 5
Expected: slaveCount
Which is: 1000
[  FAILED  ] 
SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.AddAndUpdateSlave/0,
 where GetParam() = (1000, 1) (497 ms)
[--] 1 test from 
SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test (498 ms total)

[--] Global test environment tear-down
[==] 1 test from 1 test case ran. (512 ms total)
[  PASSED  ] 0 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] 
SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.AddAndUpdateSlave/0,
 where GetParam() = (1000, 1)

 1 FAILED TEST
```

The reason is that we cannot make sure all of the `_allocate()` finished 
after `addSlave` finished.

Shall we do a `while` loop in the benchmark to wait till all allocations 
are got?


- Guangya Liu


On 九月 3, 2016, 6:05 a.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51027/
> ---
> 
> (Updated 九月 3, 2016, 6:05 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Guangya Liu, James Peach, Klaus 
> Ma, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-3157
> https://issues.apache.org/jira/browse/MESOS-3157
> 
> 
> 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.
> - Batched allocations are handled synchronously.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> dd07ed221d2c1755d2478369641ffdc46ecc4471 
>   src/master/allocator/mesos/hierarchical.cpp 
> 9e5db2196c6a541dc1208ba8b9f13ef9a518bcc4 
> 
> Diff: https://reviews.apache.org/r/51027/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> note: check without filters depends on https://reviews.apache.org/r/51028
> 
> With new benchmark https://reviews.apache.org/r/49617: 
> Sample output without 51027:
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
> Using 1 agents and 3000 frameworks
> Added 3000 frameworks in 57251us
> Added 1 agents in 3.2134535333mins
> allocator settled after  1.6123603833mins
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
>  (290578 ms)
> 
> Sample output with 51027:
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
> Using 1 agents and 3000 frameworks
> Added 3000 frameworks in 39817us
> Added 1 agents in 3.2286054167mins
> allocator settled after  25.525654secs
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
>  (220137 ms)
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

Re: Review Request 51027: Track allocation candidates to bound allocator.

[Guangya Liu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51027/#review147718
---




src/master/allocator/mesos/hierarchical.cpp (lines 1212 - 1215)


Just nit, I think that we should separate the `_allocate()` and 
`_deallocate()` to use different time elapse. But as the original logic is also 
putting the `_deallocate()` logic in `_allocate()`, so I think that it is OK to 
keep such logic but may need an update if we want to make the time more 
accurate for diffrent actions: `_allocate()` and `_deallocate()`.

```
stopwatch.start();

metrics.allocation_run.start();

_allocate();

metrics.allocation_run.stop();

stopwatch.stop();

VLOG(1) << "Performed allocation for " << allocationCandidates.size()
<< " agents in " << stopwatch.elapsed();

stopwatch.start();

_deallocate();

stopwatch.stop();

VLOG(1) << "Performed deallocation for " << allocationCandidates.size()
<< " agents in " << stopwatch.elapsed();
```


- Guangya Liu


On 九月 3, 2016, 6:05 a.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51027/
> ---
> 
> (Updated 九月 3, 2016, 6:05 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Guangya Liu, James Peach, Klaus 
> Ma, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-3157
> https://issues.apache.org/jira/browse/MESOS-3157
> 
> 
> 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.
> - Batched allocations are handled synchronously.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> dd07ed221d2c1755d2478369641ffdc46ecc4471 
>   src/master/allocator/mesos/hierarchical.cpp 
> 9e5db2196c6a541dc1208ba8b9f13ef9a518bcc4 
> 
> Diff: https://reviews.apache.org/r/51027/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> note: check without filters depends on https://reviews.apache.org/r/51028
> 
> With new benchmark https://reviews.apache.org/r/49617: 
> Sample output without 51027:
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
> Using 1 agents and 3000 frameworks
> Added 3000 frameworks in 57251us
> Added 1 agents in 3.2134535333mins
> allocator settled after  1.6123603833mins
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
>  (290578 ms)
> 
> Sample output with 51027:
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
> Using 1 agents and 3000 frameworks
> Added 3000 frameworks in 39817us
> Added 1 agents in 3.2286054167mins
> allocator settled after  25.525654secs
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
>  (220137 ms)
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

Re: Review Request 51027: Track allocation candidates to bound allocator.

[Jacob Janco]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51027/
---

(Updated Sept. 3, 2016, 6:05 a.m.)


Review request for mesos, Benjamin Mahler, Guangya Liu, James Peach, Klaus Ma, 
and Jiang Yan Xu.


Bugs: MESOS-3157
https://issues.apache.org/jira/browse/MESOS-3157


Repository: mesos


Description (updated)
---

- 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.
- Batched allocations are handled synchronously.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.hpp 
dd07ed221d2c1755d2478369641ffdc46ecc4471 
  src/master/allocator/mesos/hierarchical.cpp 
9e5db2196c6a541dc1208ba8b9f13ef9a518bcc4 

Diff: https://reviews.apache.org/r/51027/diff/


Testing
---

make check

note: check without filters depends on https://reviews.apache.org/r/51028

With new benchmark https://reviews.apache.org/r/49617: 
Sample output without 51027:
[ RUN  ] 
SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
Using 1 agents and 3000 frameworks
Added 3000 frameworks in 57251us
Added 1 agents in 3.2134535333mins
allocator settled after  1.6123603833mins
[   OK ] 
SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
 (290578 ms)

Sample output with 51027:
[ RUN  ] 
SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
Using 1 agents and 3000 frameworks
Added 3000 frameworks in 39817us
Added 1 agents in 3.2286054167mins
allocator settled after  25.525654secs
[   OK ] 
SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
 (220137 ms)


Thanks,

Jacob Janco

Re: Review Request 51027: Track allocation candidates to bound allocator.

[Jiang Yan Xu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51027/#review147674
---


Fix it, then Ship it!




Modulo comments from others and I'll look at the benchmark results before 
merging.


src/master/allocator/mesos/hierarchical.hpp (lines 219 - 223)


There is symmetry between the two so let's name / document them similarly.

```
// Helper for `allocate()` that allocates resources for offers.
void _allocate();

// Helper for `allocate()` that deallocates resources for inverse offers.
void _deallocate();
```



src/master/allocator/mesos/hierarchical.cpp (lines 1555 - 1558)


Given the way the code is structured now, I think we can just move this 
block to `allocate()`, after it calls `_allocate()`.


- Jiang Yan Xu


On Aug. 30, 2016, 9:53 a.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51027/
> ---
> 
> (Updated Aug. 30, 2016, 9:53 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Guangya Liu, James Peach, Klaus 
> Ma, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-3157
> https://issues.apache.org/jira/browse/MESOS-3157
> 
> 
> 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.
> - Batched allocations are handled synchronously.
> 
>   
> - Carrying over work from 
>   https://reviews.apache.org/r/41658/ and added 
>   the previous reviewers
> - Specifically, this patch introduces the boolean 
>   flag pendingAllocation, which when set on event 
>   triggered allocations, will prevent additional 
>   no-op allocations: the flag is cleared when the 
>   enqueued allocation is processed, subsequent 
>   event triggered allocations will update a set
>   of allocation candidates rather than dispatching 
>   an additional allocate().
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> bdbc6d3b5b959990538f4e3b7b1a3b031d9aea05 
>   src/master/allocator/mesos/hierarchical.cpp 
> 234ef98529964a0b6d3f132426a6c8ccbb1263ee 
> 
> Diff: https://reviews.apache.org/r/51027/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> note: check without filters depends on https://reviews.apache.org/r/51028
> 
> With new benchmark https://reviews.apache.org/r/49617: 
> Sample output without 51027:
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
> Using 1 agents and 3000 frameworks
> Added 3000 frameworks in 57251us
> Added 1 agents in 3.2134535333mins
> allocator settled after  1.6123603833mins
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
>  (290578 ms)
> 
> Sample output with 51027:
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
> Using 1 agents and 3000 frameworks
> Added 3000 frameworks in 39817us
> Added 1 agents in 3.2286054167mins
> allocator settled after  25.525654secs
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
>  (220137 ms)
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

Re: Review Request 51027: Track allocation candidates to bound allocator.

[Guangya Liu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51027/#review147648
---



Jacob, can you please help rebase your patch due to some patches related to 
`shared resources` are now merged in `hierarchical.cpp`? Thanks.

- Guangya Liu


On 八月 30, 2016, 4:53 p.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51027/
> ---
> 
> (Updated 八月 30, 2016, 4:53 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Guangya Liu, James Peach, Klaus 
> Ma, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-3157
> https://issues.apache.org/jira/browse/MESOS-3157
> 
> 
> 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.
> - Batched allocations are handled synchronously.
> 
>   
> - Carrying over work from 
>   https://reviews.apache.org/r/41658/ and added 
>   the previous reviewers
> - Specifically, this patch introduces the boolean 
>   flag pendingAllocation, which when set on event 
>   triggered allocations, will prevent additional 
>   no-op allocations: the flag is cleared when the 
>   enqueued allocation is processed, subsequent 
>   event triggered allocations will update a set
>   of allocation candidates rather than dispatching 
>   an additional allocate().
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> bdbc6d3b5b959990538f4e3b7b1a3b031d9aea05 
>   src/master/allocator/mesos/hierarchical.cpp 
> 234ef98529964a0b6d3f132426a6c8ccbb1263ee 
> 
> Diff: https://reviews.apache.org/r/51027/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> note: check without filters depends on https://reviews.apache.org/r/51028
> 
> With new benchmark https://reviews.apache.org/r/49617: 
> Sample output without 51027:
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
> Using 1 agents and 3000 frameworks
> Added 3000 frameworks in 57251us
> Added 1 agents in 3.2134535333mins
> allocator settled after  1.6123603833mins
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
>  (290578 ms)
> 
> Sample output with 51027:
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
> Using 1 agents and 3000 frameworks
> Added 3000 frameworks in 39817us
> Added 1 agents in 3.2286054167mins
> allocator settled after  25.525654secs
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
>  (220137 ms)
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

Re: Review Request 51027: Track allocation candidates to bound allocator.

[Jacob Janco]

> On Aug. 26, 2016, 6:24 a.m., Klaus Ma wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 275
> > 
> >
> > I think we should use `insert(...)` instead of `=`; if the following 
> > event in the queue, are we still going to get the correct result?
> > ```
> > batch -> addFramework(f1) -> addFramework(f2)
> > ```
> 
> Jacob Janco wrote:
> insert in a loop?
> 
> Klaus Ma wrote:
> No; if multiple frameworks register at almost the same time, will there 
> be multiple addFramework events pending in the queue? Did not get a chance to 
> test it, but did we consider such kind of race condition?

If I understand this correctly: batch -> addFramework(f1) -> addFramework(f2) 
will ensure an allocate() at batch over all known slaves. If the two 
addFramework events come in close succession then the queue should look like 1: 
addFramework(f1) -> addFramework(f2) 2: addFramework(f2) -> allocate() 3: 
allocate() There will be a pendingAllocation set for the first event with the 
dispatched allocate() after addFramework(f2). I'm not sure what the race 
condition would be as we can have many addFramework events queued.


- Jacob


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51027/#review146919
---


On Aug. 30, 2016, 4:53 p.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51027/
> ---
> 
> (Updated Aug. 30, 2016, 4:53 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Guangya Liu, James Peach, Klaus 
> Ma, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-3157
> https://issues.apache.org/jira/browse/MESOS-3157
> 
> 
> 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.
> - Batched allocations are handled synchronously.
> 
>   
> - Carrying over work from 
>   https://reviews.apache.org/r/41658/ and added 
>   the previous reviewers
> - Specifically, this patch introduces the boolean 
>   flag pendingAllocation, which when set on event 
>   triggered allocations, will prevent additional 
>   no-op allocations: the flag is cleared when the 
>   enqueued allocation is processed, subsequent 
>   event triggered allocations will update a set
>   of allocation candidates rather than dispatching 
>   an additional allocate().
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> bdbc6d3b5b959990538f4e3b7b1a3b031d9aea05 
>   src/master/allocator/mesos/hierarchical.cpp 
> 234ef98529964a0b6d3f132426a6c8ccbb1263ee 
> 
> Diff: https://reviews.apache.org/r/51027/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> note: check without filters depends on https://reviews.apache.org/r/51028
> 
> With new benchmark https://reviews.apache.org/r/49617: 
> Sample output without 51027:
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
> Using 1 agents and 3000 frameworks
> Added 3000 frameworks in 57251us
> Added 1 agents in 3.2134535333mins
> allocator settled after  1.6123603833mins
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
>  (290578 ms)
> 
> Sample output with 51027:
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
> Using 1 agents and 3000 frameworks
> Added 3000 frameworks in 39817us
> Added 1 agents in 3.2286054167mins
> allocator settled after  25.525654secs
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
>  (220137 ms)
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

Re: Review Request 51027: Track allocation candidates to bound allocator.

[Klaus Ma]

> On Aug. 26, 2016, 2:24 p.m., Klaus Ma wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 275
> > 
> >
> > I think we should use `insert(...)` instead of `=`; if the following 
> > event in the queue, are we still going to get the correct result?
> > ```
> > batch -> addFramework(f1) -> addFramework(f2)
> > ```
> 
> Jacob Janco wrote:
> insert in a loop?

No; if multiple frameworks register at almost the same time, will there be 
multiple addFramework events pending in the queue? Did not get a chance to test 
it, but did we consider such kind of race condition?


- Klaus


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51027/#review146919
---


On Aug. 31, 2016, 12:53 a.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51027/
> ---
> 
> (Updated Aug. 31, 2016, 12:53 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Guangya Liu, James Peach, Klaus 
> Ma, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-3157
> https://issues.apache.org/jira/browse/MESOS-3157
> 
> 
> 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.
> - Batched allocations are handled synchronously.
> 
>   
> - Carrying over work from 
>   https://reviews.apache.org/r/41658/ and added 
>   the previous reviewers
> - Specifically, this patch introduces the boolean 
>   flag pendingAllocation, which when set on event 
>   triggered allocations, will prevent additional 
>   no-op allocations: the flag is cleared when the 
>   enqueued allocation is processed, subsequent 
>   event triggered allocations will update a set
>   of allocation candidates rather than dispatching 
>   an additional allocate().
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> bdbc6d3b5b959990538f4e3b7b1a3b031d9aea05 
>   src/master/allocator/mesos/hierarchical.cpp 
> 234ef98529964a0b6d3f132426a6c8ccbb1263ee 
> 
> Diff: https://reviews.apache.org/r/51027/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> note: check without filters depends on https://reviews.apache.org/r/51028
> 
> With new benchmark https://reviews.apache.org/r/49617: 
> Sample output without 51027:
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
> Using 1 agents and 3000 frameworks
> Added 3000 frameworks in 57251us
> Added 1 agents in 3.2134535333mins
> allocator settled after  1.6123603833mins
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
>  (290578 ms)
> 
> Sample output with 51027:
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
> Using 1 agents and 3000 frameworks
> Added 3000 frameworks in 39817us
> Added 1 agents in 3.2286054167mins
> allocator settled after  25.525654secs
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
>  (220137 ms)
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

Re: Review Request 51027: Track allocation candidates to bound allocator.

[Mesos ReviewBot]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51027/#review147333
---



Patch looks great!

Reviews applied: [51028, 51027]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker_build.sh

- Mesos ReviewBot


On Aug. 30, 2016, 4:53 p.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51027/
> ---
> 
> (Updated Aug. 30, 2016, 4:53 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Guangya Liu, James Peach, Klaus 
> Ma, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-3157
> https://issues.apache.org/jira/browse/MESOS-3157
> 
> 
> 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.
> - Batched allocations are handled synchronously.
> 
>   
> - Carrying over work from 
>   https://reviews.apache.org/r/41658/ and added 
>   the previous reviewers
> - Specifically, this patch introduces the boolean 
>   flag pendingAllocation, which when set on event 
>   triggered allocations, will prevent additional 
>   no-op allocations: the flag is cleared when the 
>   enqueued allocation is processed, subsequent 
>   event triggered allocations will update a set
>   of allocation candidates rather than dispatching 
>   an additional allocate().
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> bdbc6d3b5b959990538f4e3b7b1a3b031d9aea05 
>   src/master/allocator/mesos/hierarchical.cpp 
> 234ef98529964a0b6d3f132426a6c8ccbb1263ee 
> 
> Diff: https://reviews.apache.org/r/51027/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> note: check without filters depends on https://reviews.apache.org/r/51028
> 
> With new benchmark https://reviews.apache.org/r/49617: 
> Sample output without 51027:
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
> Using 1 agents and 3000 frameworks
> Added 3000 frameworks in 57251us
> Added 1 agents in 3.2134535333mins
> allocator settled after  1.6123603833mins
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
>  (290578 ms)
> 
> Sample output with 51027:
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
> Using 1 agents and 3000 frameworks
> Added 3000 frameworks in 39817us
> Added 1 agents in 3.2286054167mins
> allocator settled after  25.525654secs
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
>  (220137 ms)
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

Re: Review Request 51027: Track allocation candidates to bound allocator.

[Guangya Liu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51027/#review147321
---




src/master/allocator/mesos/hierarchical.hpp (line 219)


s/allocate()/`allocate()`



src/master/allocator/mesos/hierarchical.hpp (line 222)


The comments should be updated as 

```
Send inverse offers from the allocation candidates.
```



src/master/allocator/mesos/hierarchical.hpp (lines 225 - 226)


Make this less jagged.

```
// Ensure an allocation run is pending in the event
// queue. An allocate will be dispatched otherwise.
```



src/master/allocator/mesos/hierarchical.hpp (lines 387 - 389)


```
// A set of agents that are kept as allocation candidates. Events
// triggering allocations will add or remove candidates to the set.
// When an allocation is processed, the set of candidates is cleared.
```


- Guangya Liu


On 八月 30, 2016, 4:53 p.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51027/
> ---
> 
> (Updated 八月 30, 2016, 4:53 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Guangya Liu, James Peach, Klaus 
> Ma, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-3157
> https://issues.apache.org/jira/browse/MESOS-3157
> 
> 
> 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.
> - Batched allocations are handled synchronously.
> 
>   
> - Carrying over work from 
>   https://reviews.apache.org/r/41658/ and added 
>   the previous reviewers
> - Specifically, this patch introduces the boolean 
>   flag pendingAllocation, which when set on event 
>   triggered allocations, will prevent additional 
>   no-op allocations: the flag is cleared when the 
>   enqueued allocation is processed, subsequent 
>   event triggered allocations will update a set
>   of allocation candidates rather than dispatching 
>   an additional allocate().
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> bdbc6d3b5b959990538f4e3b7b1a3b031d9aea05 
>   src/master/allocator/mesos/hierarchical.cpp 
> 234ef98529964a0b6d3f132426a6c8ccbb1263ee 
> 
> Diff: https://reviews.apache.org/r/51027/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> note: check without filters depends on https://reviews.apache.org/r/51028
> 
> With new benchmark https://reviews.apache.org/r/49617: 
> Sample output without 51027:
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
> Using 1 agents and 3000 frameworks
> Added 3000 frameworks in 57251us
> Added 1 agents in 3.2134535333mins
> allocator settled after  1.6123603833mins
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
>  (290578 ms)
> 
> Sample output with 51027:
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
> Using 1 agents and 3000 frameworks
> Added 3000 frameworks in 39817us
> Added 1 agents in 3.2286054167mins
> allocator settled after  25.525654secs
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
>  (220137 ms)
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

Re: Review Request 51027: Track allocation candidates to bound allocator.

[Jacob Janco]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51027/
---

(Updated Aug. 30, 2016, 4:50 p.m.)


Review request for mesos, Benjamin Mahler, Guangya Liu, James Peach, Klaus Ma, 
and Jiang Yan Xu.


Bugs: MESOS-3157
https://issues.apache.org/jira/browse/MESOS-3157


Repository: mesos


Description (updated)
---

- 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.
- Batched allocations are handled synchronously.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.hpp 
bdbc6d3b5b959990538f4e3b7b1a3b031d9aea05 
  src/master/allocator/mesos/hierarchical.cpp 
234ef98529964a0b6d3f132426a6c8ccbb1263ee 

Diff: https://reviews.apache.org/r/51027/diff/


Testing
---

make check

note: check without filters depends on https://reviews.apache.org/r/51028

With new benchmark https://reviews.apache.org/r/49617: 
Sample output without 51027:
[ RUN  ] 
SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
Using 1 agents and 3000 frameworks
Added 3000 frameworks in 57251us
Added 1 agents in 3.2134535333mins
allocator settled after  1.6123603833mins
[   OK ] 
SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
 (290578 ms)

Sample output with 51027:
[ RUN  ] 
SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
Using 1 agents and 3000 frameworks
Added 3000 frameworks in 39817us
Added 1 agents in 3.2286054167mins
allocator settled after  25.525654secs
[   OK ] 
SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
 (220137 ms)


Thanks,

Jacob Janco

Re: Review Request 51027: Track allocation candidates to bound allocator.

[Jacob Janco]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51027/
---

(Updated Aug. 30, 2016, 4:53 p.m.)


Review request for mesos, Benjamin Mahler, Guangya Liu, James Peach, Klaus Ma, 
and Jiang Yan Xu.


Bugs: MESOS-3157
https://issues.apache.org/jira/browse/MESOS-3157


Repository: mesos


Description (updated)
---

- 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.
- Batched allocations are handled synchronously.


- Carrying over work from 
  https://reviews.apache.org/r/41658/ and added 
  the previous reviewers
- Specifically, this patch introduces the boolean 
  flag pendingAllocation, which when set on event 
  triggered allocations, will prevent additional 
  no-op allocations: the flag is cleared when the 
  enqueued allocation is processed, subsequent 
  event triggered allocations will update a set
  of allocation candidates rather than dispatching 
  an additional allocate().


Diffs
-

  src/master/allocator/mesos/hierarchical.hpp 
bdbc6d3b5b959990538f4e3b7b1a3b031d9aea05 
  src/master/allocator/mesos/hierarchical.cpp 
234ef98529964a0b6d3f132426a6c8ccbb1263ee 

Diff: https://reviews.apache.org/r/51027/diff/


Testing
---

make check

note: check without filters depends on https://reviews.apache.org/r/51028

With new benchmark https://reviews.apache.org/r/49617: 
Sample output without 51027:
[ RUN  ] 
SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
Using 1 agents and 3000 frameworks
Added 3000 frameworks in 57251us
Added 1 agents in 3.2134535333mins
allocator settled after  1.6123603833mins
[   OK ] 
SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
 (290578 ms)

Sample output with 51027:
[ RUN  ] 
SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
Using 1 agents and 3000 frameworks
Added 3000 frameworks in 39817us
Added 1 agents in 3.2286054167mins
allocator settled after  25.525654secs
[   OK ] 
SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
 (220137 ms)


Thanks,

Jacob Janco

Re: Review Request 51027: Track allocation candidates to bound allocator.

[Jacob Janco]

> On Aug. 26, 2016, 6:24 a.m., Klaus Ma wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 275
> > 
> >
> > I think we should use `insert(...)` instead of `=`; if the following 
> > event in the queue, are we still going to get the correct result?
> > ```
> > batch -> addFramework(f1) -> addFramework(f2)
> > ```

insert in a loop?


- Jacob


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51027/#review146919
---


On Aug. 23, 2016, 8:49 a.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51027/
> ---
> 
> (Updated Aug. 23, 2016, 8:49 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Guangya Liu, James Peach, Klaus 
> Ma, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-3157
> https://issues.apache.org/jira/browse/MESOS-3157
> 
> 
> 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.
> - Batched allocations are handled synchronously.
> 
> - Carrying over work from https://reviews.apache.org/r/41658/ and added the 
> previous reviewers
> - Specifically, this patch introduces the boolean flag pendingAllocation, 
> which when set on event 
>   triggered allocations, will prevent additional no-op allocations: the flag 
> is cleared when 
>   the enqueued allocation is processed, subsequent event triggered 
> allocations will update a set
>   of allocation candidates rather than dispatching an additional allocate().
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> bdbc6d3b5b959990538f4e3b7b1a3b031d9aea05 
>   src/master/allocator/mesos/hierarchical.cpp 
> 234ef98529964a0b6d3f132426a6c8ccbb1263ee 
> 
> Diff: https://reviews.apache.org/r/51027/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> note: check without filters depends on https://reviews.apache.org/r/51028
> 
> With new benchmark https://reviews.apache.org/r/49617: 
> Sample output without 51027:
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
> Using 1 agents and 3000 frameworks
> Added 3000 frameworks in 57251us
> Added 1 agents in 3.2134535333mins
> allocator settled after  1.6123603833mins
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
>  (290578 ms)
> 
> Sample output with 51027:
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
> Using 1 agents and 3000 frameworks
> Added 3000 frameworks in 39817us
> Added 1 agents in 3.2286054167mins
> allocator settled after  25.525654secs
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
>  (220137 ms)
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

Re: Review Request 51027: Track allocation candidates to bound allocator.

[Klaus Ma]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51027/#review146919
---




src/master/allocator/mesos/hierarchical.cpp (line 274)


I think we should use `insert(...)` instead of `=`; if the following event 
in the queue, are we still going to get the correct result?
```
batch -> addFramework(f1) -> addFramework(f2)
```


- Klaus Ma


On Aug. 23, 2016, 4:49 p.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51027/
> ---
> 
> (Updated Aug. 23, 2016, 4:49 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Guangya Liu, James Peach, Klaus 
> Ma, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-3157
> https://issues.apache.org/jira/browse/MESOS-3157
> 
> 
> 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.
> - Batched allocations are handled synchronously.
> 
> - Carrying over work from https://reviews.apache.org/r/41658/ and added the 
> previous reviewers
> - Specifically, this patch introduces the boolean flag pendingAllocation, 
> which when set on event 
>   triggered allocations, will prevent additional no-op allocations: the flag 
> is cleared when 
>   the enqueued allocation is processed, subsequent event triggered 
> allocations will update a set
>   of allocation candidates rather than dispatching an additional allocate().
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> bdbc6d3b5b959990538f4e3b7b1a3b031d9aea05 
>   src/master/allocator/mesos/hierarchical.cpp 
> 234ef98529964a0b6d3f132426a6c8ccbb1263ee 
> 
> Diff: https://reviews.apache.org/r/51027/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> note: check without filters depends on https://reviews.apache.org/r/51028
> 
> With new benchmark https://reviews.apache.org/r/49617: 
> Sample output without 51027:
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
> Using 1 agents and 3000 frameworks
> Added 3000 frameworks in 57251us
> Added 1 agents in 3.2134535333mins
> allocator settled after  1.6123603833mins
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
>  (290578 ms)
> 
> Sample output with 51027:
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
> Using 1 agents and 3000 frameworks
> Added 3000 frameworks in 39817us
> Added 1 agents in 3.2286054167mins
> allocator settled after  25.525654secs
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
>  (220137 ms)
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

Re: Review Request 51027: Track allocation candidates to bound allocator.

[Jiang Yan Xu]

> On Aug. 23, 2016, 2:26 a.m., Alexander Rukletsov wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 276-279
> > 
> >
> > Probably extract this snippet in a function, e.g. 
> > `conditionalAllocate()`?
> 
> Jiang Yan Xu wrote:
> I thought about it but was struggling to find a short and clear method 
> name. So to describe the function in a full sentense it's "dispatch an 
> allocate() call if the condition `!allocationPending` is met". I think 
> `conditionalAllocate()` is OK but not great, it's not clear what the 
> condition is and not clear about the dispatch. I agree it's worth doing if we 
> can abstract this out without needing to explain what each line of a 4-line 
> method is doing in the comment...
> 
> Alexander Rukletsov wrote:
> Let's focus on _why_ instead of _how_. You want to ensure that one and 
> only one event allocation happens after the event. Dispatch+flag is _how_ you 
> achieve this, which may change in future. Here are some suggestions:
> - `ensureAllocate`
> - `ensureAllocation`
> - `allocateOnce`
> 
> Side note: "unique dispatch", i.e. a dispatch that succeeds only if there 
> are no identical messages in the actor's mailbox, looks like something we may 
> need in other places. Maybe we should consider adding such functionality into 
> libprocess.

Good point. I like `ensureAllocation()` better than the rest.

```
// Ensure an allocation run is pending in the event queue (i.e,. one is 
dispatched otherwise).
// TODO: Consider generalizing this into a "unique dispatch" functionality in 
libprocess.
void ensureAllocation();
```

Yeah I like the idea of "unique dispatch", see the TODO.


- Jiang Yan


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51027/#review146499
---


On Aug. 23, 2016, 1:49 a.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51027/
> ---
> 
> (Updated Aug. 23, 2016, 1:49 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Guangya Liu, James Peach, Klaus 
> Ma, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-3157
> https://issues.apache.org/jira/browse/MESOS-3157
> 
> 
> 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.
> - Batched allocations are handled synchronously.
> 
> - Carrying over work from https://reviews.apache.org/r/41658/ and added the 
> previous reviewers
> - Specifically, this patch introduces the boolean flag pendingAllocation, 
> which when set on event 
>   triggered allocations, will prevent additional no-op allocations: the flag 
> is cleared when 
>   the enqueued allocation is processed, subsequent event triggered 
> allocations will update a set
>   of allocation candidates rather than dispatching an additional allocate().
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> bdbc6d3b5b959990538f4e3b7b1a3b031d9aea05 
>   src/master/allocator/mesos/hierarchical.cpp 
> 234ef98529964a0b6d3f132426a6c8ccbb1263ee 
> 
> Diff: https://reviews.apache.org/r/51027/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> note: check without filters depends on https://reviews.apache.org/r/51028
> 
> With new benchmark https://reviews.apache.org/r/49617: 
> Sample output without 51027:
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
> Using 1 agents and 3000 frameworks
> Added 3000 frameworks in 57251us
> Added 1 agents in 3.2134535333mins
> allocator settled after  1.6123603833mins
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
>  (290578 ms)
> 
> Sample output with 51027:
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
> Using 1 agents and 3000 frameworks
> Added 3000 frameworks in 39817us
> Added 1 agents in 3.2286054167mins
> allocator settled after  25.525654secs
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
>  (220137 ms)
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

Re: Review Request 51027: Track allocation candidates to bound allocator.

[Mesos ReviewBot]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51027/#review146673
---



Patch looks great!

Reviews applied: [51028, 51027]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker_build.sh

- Mesos ReviewBot


On Aug. 23, 2016, 8:49 a.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51027/
> ---
> 
> (Updated Aug. 23, 2016, 8:49 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Guangya Liu, James Peach, Klaus 
> Ma, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-3157
> https://issues.apache.org/jira/browse/MESOS-3157
> 
> 
> 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.
> - Batched allocations are handled synchronously.
> 
> - Carrying over work from https://reviews.apache.org/r/41658/ and added the 
> previous reviewers
> - Specifically, this patch introduces the boolean flag pendingAllocation, 
> which when set on event 
>   triggered allocations, will prevent additional no-op allocations: the flag 
> is cleared when 
>   the enqueued allocation is processed, subsequent event triggered 
> allocations will update a set
>   of allocation candidates rather than dispatching an additional allocate().
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> bdbc6d3b5b959990538f4e3b7b1a3b031d9aea05 
>   src/master/allocator/mesos/hierarchical.cpp 
> 234ef98529964a0b6d3f132426a6c8ccbb1263ee 
> 
> Diff: https://reviews.apache.org/r/51027/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> note: check without filters depends on https://reviews.apache.org/r/51028
> 
> With new benchmark https://reviews.apache.org/r/49617: 
> Sample output without 51027:
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
> Using 1 agents and 3000 frameworks
> Added 3000 frameworks in 57251us
> Added 1 agents in 3.2134535333mins
> allocator settled after  1.6123603833mins
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
>  (290578 ms)
> 
> Sample output with 51027:
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
> Using 1 agents and 3000 frameworks
> Added 3000 frameworks in 39817us
> Added 1 agents in 3.2286054167mins
> allocator settled after  25.525654secs
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
>  (220137 ms)
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

Re: Review Request 51027: Track allocation candidates to bound allocator.

[Alexander Rukletsov]

> On Aug. 23, 2016, 9:26 a.m., Alexander Rukletsov wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 1194-1195
> > 
> >
> > `delay` already contains `dispatch`, so under "synchronously" you 
> > actually mean double dispatch.
> 
> Jiang Yan Xu wrote:
> I originally suggested putting this comment inside `batch()` and directly 
> above `allocate()` so it's more clear what `synchronously` applies to: 
> calling `allocate()` without dispatching.
> 
> *delay* in this sentence doesn't mean the `delay()` call but in the 
> literal sense. To make it more clear, how about we say:
> 
> ```
> // We run the allocation synchronously here instead of dispatching it 
> **again** so
> // a batched allocation **doesn't lag behind further** if the allocator 
> is backed up.
> ```
> 
> Note the asterisks are just to emphasize the changes.

This reads better!


> On Aug. 23, 2016, 9:26 a.m., Alexander Rukletsov wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 276-279
> > 
> >
> > Probably extract this snippet in a function, e.g. 
> > `conditionalAllocate()`?
> 
> Jiang Yan Xu wrote:
> I thought about it but was struggling to find a short and clear method 
> name. So to describe the function in a full sentense it's "dispatch an 
> allocate() call if the condition `!allocationPending` is met". I think 
> `conditionalAllocate()` is OK but not great, it's not clear what the 
> condition is and not clear about the dispatch. I agree it's worth doing if we 
> can abstract this out without needing to explain what each line of a 4-line 
> method is doing in the comment...

Let's focus on _why_ instead of _how_. You want to ensure that one and only one 
event allocation happens after the event. Dispatch+flag is _how_ you achieve 
this, which may change in future. Here are some suggestions:
- `ensureAllocate`
- `ensureAllocation`
- `allocateOnce`

Side note: "unique dispatch", i.e. a dispatch that succeeds only if there are 
no identical messages in the actor's mailbox, looks like something we may need 
in other places. Maybe we should consider adding such functionality into 
libprocess.


- Alexander


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51027/#review146499
---


On Aug. 23, 2016, 8:49 a.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51027/
> ---
> 
> (Updated Aug. 23, 2016, 8:49 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Guangya Liu, James Peach, Klaus 
> Ma, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-3157
> https://issues.apache.org/jira/browse/MESOS-3157
> 
> 
> 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.
> - Batched allocations are handled synchronously.
> 
> - Carrying over work from https://reviews.apache.org/r/41658/ and added the 
> previous reviewers
> - Specifically, this patch introduces the boolean flag pendingAllocation, 
> which when set on event 
>   triggered allocations, will prevent additional no-op allocations: the flag 
> is cleared when 
>   the enqueued allocation is processed, subsequent event triggered 
> allocations will update a set
>   of allocation candidates rather than dispatching an additional allocate().
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> bdbc6d3b5b959990538f4e3b7b1a3b031d9aea05 
>   src/master/allocator/mesos/hierarchical.cpp 
> 234ef98529964a0b6d3f132426a6c8ccbb1263ee 
> 
> Diff: https://reviews.apache.org/r/51027/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> note: check without filters depends on https://reviews.apache.org/r/51028
> 
> With new benchmark https://reviews.apache.org/r/49617: 
> Sample output without 51027:
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
> Using 1 agents and 3000 frameworks
> Added 3000 frameworks in 57251us
> Added 1 agents in 3.2134535333mins
> allocator settled after  1.6123603833mins
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
>  (290578 ms)
> 
> Sample output with 51027:
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
> Using 1 agents and 3000 frameworks
> Added 3000 frameworks in 39817us
> Added 1 agents in 3.2286054167mins
> 

Re: Review Request 51027: Track allocation candidates to bound allocator.

[Guangya Liu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51027/#review146620
---



@Jacob, can you please add more test cases in `Testing Done` section? Such as 
some comparision as my above append.

Before your patch:
```
Test result and data
```

After your patch:
```
Test result and data
```

So that people can know how much improvment does your patch made for allocator.

- Guangya Liu


On 八月 23, 2016, 8:49 a.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51027/
> ---
> 
> (Updated 八月 23, 2016, 8:49 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Guangya Liu, James Peach, Klaus 
> Ma, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-3157
> https://issues.apache.org/jira/browse/MESOS-3157
> 
> 
> 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.
> - Batched allocations are handled synchronously.
> 
> - Carrying over work from https://reviews.apache.org/r/41658/ and added the 
> previous reviewers
> - Specifically, this patch introduces the boolean flag pendingAllocation, 
> which when set on event 
>   triggered allocations, will prevent additional no-op allocations: the flag 
> is cleared when 
>   the enqueued allocation is processed, subsequent event triggered 
> allocations will update a set
>   of allocation candidates rather than dispatching an additional allocate().
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> bdbc6d3b5b959990538f4e3b7b1a3b031d9aea05 
>   src/master/allocator/mesos/hierarchical.cpp 
> 234ef98529964a0b6d3f132426a6c8ccbb1263ee 
> 
> Diff: https://reviews.apache.org/r/51027/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> note: check without filters depends on https://reviews.apache.org/r/51028
> 
> With new benchmark https://reviews.apache.org/r/49617: 
> Sample output without 51027:
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
> Using 1 agents and 3000 frameworks
> Added 3000 frameworks in 57251us
> Added 1 agents in 3.2134535333mins
> allocator settled after  1.6123603833mins
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
>  (290578 ms)
> 
> Sample output with 51027:
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
> Using 1 agents and 3000 frameworks
> Added 3000 frameworks in 39817us
> Added 1 agents in 3.2286054167mins
> allocator settled after  25.525654secs
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
>  (220137 ms)
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

Re: Review Request 51027: Track allocation candidates to bound allocator.

[Guangya Liu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51027/#review146591
---



This patch really improved the performance a lot for allocator, I did some test 
with 6000 frameworks and 1000 agents (Add 6000 frameworks first then add 1000 
agents), the result is quite promising! Please take a look at the result of 
adding 1000 agents, the time decreased from `4.6 mins` to `49 secs`! It would 
help a lot for master failover with thousands of frameworks and agents.

Before fix:
```
[ RUN  ] 
SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.DeclineOffers/7
Using 1000 agents and 6000 frameworks
Added 6000 frameworks in 702547us
Added 1000 agents in 4.6021463667mins
```
After fix:
```
[ RUN  ] 
SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.DeclineOffers/7
Using 1000 agents and 6000 frameworks
Added 6000 frameworks in 89707us
Added 1000 agents in 49.351196secs
```

- Guangya Liu


On 八月 23, 2016, 8:49 a.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51027/
> ---
> 
> (Updated 八月 23, 2016, 8:49 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Guangya Liu, James Peach, Klaus 
> Ma, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-3157
> https://issues.apache.org/jira/browse/MESOS-3157
> 
> 
> 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.
> - Batched allocations are handled synchronously.
> 
> - Carrying over work from https://reviews.apache.org/r/41658/ and added the 
> previous reviewers
> - Specifically, this patch introduces the boolean flag pendingAllocation, 
> which when set on event 
>   triggered allocations, will prevent additional no-op allocations: the flag 
> is cleared when 
>   the enqueued allocation is processed, subsequent event triggered 
> allocations will update a set
>   of allocation candidates rather than dispatching an additional allocate().
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> bdbc6d3b5b959990538f4e3b7b1a3b031d9aea05 
>   src/master/allocator/mesos/hierarchical.cpp 
> 234ef98529964a0b6d3f132426a6c8ccbb1263ee 
> 
> Diff: https://reviews.apache.org/r/51027/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> note: check without filters depends on https://reviews.apache.org/r/51028
> 
> With new benchmark https://reviews.apache.org/r/49617: 
> Sample output without 51027:
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
> Using 1 agents and 3000 frameworks
> Added 3000 frameworks in 57251us
> Added 1 agents in 3.2134535333mins
> allocator settled after  1.6123603833mins
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
>  (290578 ms)
> 
> Sample output with 51027:
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
> Using 1 agents and 3000 frameworks
> Added 3000 frameworks in 39817us
> Added 1 agents in 3.2286054167mins
> allocator settled after  25.525654secs
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
>  (220137 ms)
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

Re: Review Request 51027: Track allocation candidates to bound allocator.

[Jiang Yan Xu]

> On Aug. 23, 2016, 2:26 a.m., Alexander Rukletsov wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 1194-1195
> > 
> >
> > `delay` already contains `dispatch`, so under "synchronously" you 
> > actually mean double dispatch.

I originally suggested putting this comment inside `batch()` and directly above 
`allocate()` so it's more clear what `synchronously` applies to: calling 
`allocate()` without dispatching.

*delay* in this sentence doesn't mean the `delay()` call but in the literal 
sense. To make it more clear, how about we say:

```
// We run the allocation synchronously here instead of dispatching it **again** 
so
// a batched allocation **doesn't lag behind further** if the allocator is 
backed up.
```

Note the asterisks are just to emphasize the changes.


> On Aug. 23, 2016, 2:26 a.m., Alexander Rukletsov wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 276-279
> > 
> >
> > Probably extract this snippet in a function, e.g. 
> > `conditionalAllocate()`?

I thought about it but was struggling to find a short and clear method name. So 
to describe the function in a full sentense it's "dispatch an allocate() call 
if the condition `!allocationPending` is met". I think `conditionalAllocate()` 
is OK but not great, it's not clear what the condition is and not clear about 
the dispatch. I agree it's worth doing if we can abstract this out without 
needing to explain what each line of a 4-line method is doing in the comment...


- Jiang Yan


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51027/#review146499
---


On Aug. 23, 2016, 1:49 a.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51027/
> ---
> 
> (Updated Aug. 23, 2016, 1:49 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Guangya Liu, James Peach, Klaus 
> Ma, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-3157
> https://issues.apache.org/jira/browse/MESOS-3157
> 
> 
> 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.
> - Batched allocations are handled synchronously.
> 
> - Carrying over work from https://reviews.apache.org/r/41658/ and added the 
> previous reviewers
> - Specifically, this patch introduces the boolean flag pendingAllocation, 
> which when set on event 
>   triggered allocations, will prevent additional no-op allocations: the flag 
> is cleared when 
>   the enqueued allocation is processed, subsequent event triggered 
> allocations will update a set
>   of allocation candidates rather than dispatching an additional allocate().
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> bdbc6d3b5b959990538f4e3b7b1a3b031d9aea05 
>   src/master/allocator/mesos/hierarchical.cpp 
> 234ef98529964a0b6d3f132426a6c8ccbb1263ee 
> 
> Diff: https://reviews.apache.org/r/51027/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> note: check without filters depends on https://reviews.apache.org/r/51028
> 
> With new benchmark https://reviews.apache.org/r/49617: 
> Sample output without 51027:
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
> Using 1 agents and 3000 frameworks
> Added 3000 frameworks in 57251us
> Added 1 agents in 3.2134535333mins
> allocator settled after  1.6123603833mins
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
>  (290578 ms)
> 
> Sample output with 51027:
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
> Using 1 agents and 3000 frameworks
> Added 3000 frameworks in 39817us
> Added 1 agents in 3.2286054167mins
> allocator settled after  25.525654secs
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
>  (220137 ms)
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

Re: Review Request 51027: Track allocation candidates to bound allocator.

[Guangya Liu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51027/#review146588
---




src/master/allocator/mesos/hierarchical.hpp (line 380)


s/are/that are



src/master/allocator/mesos/hierarchical.cpp (lines 782 - 783)


What about adding a comment similar as `setQuota`?

```
// Trigger the allocation synchronously in order to promptly
// react to the operator's host maintain request.
```


- Guangya Liu


On 八月 23, 2016, 8:49 a.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51027/
> ---
> 
> (Updated 八月 23, 2016, 8:49 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Guangya Liu, James Peach, Klaus 
> Ma, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-3157
> https://issues.apache.org/jira/browse/MESOS-3157
> 
> 
> 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.
> - Batched allocations are handled synchronously.
> 
> - Carrying over work from https://reviews.apache.org/r/41658/ and added the 
> previous reviewers
> - Specifically, this patch introduces the boolean flag pendingAllocation, 
> which when set on event 
>   triggered allocations, will prevent additional no-op allocations: the flag 
> is cleared when 
>   the enqueued allocation is processed, subsequent event triggered 
> allocations will update a set
>   of allocation candidates rather than dispatching an additional allocate().
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> bdbc6d3b5b959990538f4e3b7b1a3b031d9aea05 
>   src/master/allocator/mesos/hierarchical.cpp 
> 234ef98529964a0b6d3f132426a6c8ccbb1263ee 
> 
> Diff: https://reviews.apache.org/r/51027/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> note: check without filters depends on https://reviews.apache.org/r/51028
> 
> With new benchmark https://reviews.apache.org/r/49617: 
> Sample output without 51027:
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
> Using 1 agents and 3000 frameworks
> Added 3000 frameworks in 57251us
> Added 1 agents in 3.2134535333mins
> allocator settled after  1.6123603833mins
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
>  (290578 ms)
> 
> Sample output with 51027:
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
> Using 1 agents and 3000 frameworks
> Added 3000 frameworks in 39817us
> Added 1 agents in 3.2286054167mins
> allocator settled after  25.525654secs
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
>  (220137 ms)
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

Re: Review Request 51027: Track allocation candidates to bound allocator.

[Alexander Rukletsov]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51027/#review146499
---




src/master/allocator/mesos/hierarchical.cpp (lines 275 - 278)


Probably extract this snippet in a function, e.g. `conditionalAllocate()`?



src/master/allocator/mesos/hierarchical.cpp (lines 1193 - 1194)


`delay` already contains `dispatch`, so under "synchronously" you actually 
mean double dispatch.



src/master/allocator/mesos/hierarchical.cpp (line 1200)


Remove extra blank line


- Alexander Rukletsov


On Aug. 23, 2016, 8:49 a.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51027/
> ---
> 
> (Updated Aug. 23, 2016, 8:49 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Guangya Liu, James Peach, Klaus 
> Ma, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-3157
> https://issues.apache.org/jira/browse/MESOS-3157
> 
> 
> 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.
> - Batched allocations are handled synchronously.
> 
> - Carrying over work from https://reviews.apache.org/r/41658/ and added the 
> previous reviewers
> - Specifically, this patch introduces the boolean flag pendingAllocation, 
> which when set on event 
>   triggered allocations, will prevent additional no-op allocations: the flag 
> is cleared when 
>   the enqueued allocation is processed, subsequent event triggered 
> allocations will update a set
>   of allocation candidates rather than dispatching an additional allocate().
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> bdbc6d3b5b959990538f4e3b7b1a3b031d9aea05 
>   src/master/allocator/mesos/hierarchical.cpp 
> 234ef98529964a0b6d3f132426a6c8ccbb1263ee 
> 
> Diff: https://reviews.apache.org/r/51027/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> note: check without filters depends on https://reviews.apache.org/r/51028
> 
> With new benchmark https://reviews.apache.org/r/49617: 
> Sample output without 51027:
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
> Using 1 agents and 3000 frameworks
> Added 3000 frameworks in 57251us
> Added 1 agents in 3.2134535333mins
> allocator settled after  1.6123603833mins
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
>  (290578 ms)
> 
> Sample output with 51027:
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
> Using 1 agents and 3000 frameworks
> Added 3000 frameworks in 39817us
> Added 1 agents in 3.2286054167mins
> allocator settled after  25.525654secs
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
>  (220137 ms)
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

Re: Review Request 51027: Track allocation candidates to bound allocator.

[Guangya Liu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51027/#review146497
---



You may want to update the `Description` section to make sure each line does 
not exceed 72 characters, otherwise, we cannot apply your patch.

- Guangya Liu


On 八月 23, 2016, 8:49 a.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51027/
> ---
> 
> (Updated 八月 23, 2016, 8:49 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Guangya Liu, James Peach, Klaus 
> Ma, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-3157
> https://issues.apache.org/jira/browse/MESOS-3157
> 
> 
> 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.
> - Batched allocations are handled synchronously.
> 
> - Carrying over work from https://reviews.apache.org/r/41658/ and added the 
> previous reviewers
> - Specifically, this patch introduces the boolean flag pendingAllocation, 
> which when set on event 
>   triggered allocations, will prevent additional no-op allocations: the flag 
> is cleared when 
>   the enqueued allocation is processed, subsequent event triggered 
> allocations will update a set
>   of allocation candidates rather than dispatching an additional allocate().
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> bdbc6d3b5b959990538f4e3b7b1a3b031d9aea05 
>   src/master/allocator/mesos/hierarchical.cpp 
> 234ef98529964a0b6d3f132426a6c8ccbb1263ee 
> 
> Diff: https://reviews.apache.org/r/51027/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> note: check without filters depends on https://reviews.apache.org/r/51028
> 
> With new benchmark https://reviews.apache.org/r/49617: 
> Sample output without 51027:
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
> Using 1 agents and 3000 frameworks
> Added 3000 frameworks in 57251us
> Added 1 agents in 3.2134535333mins
> allocator settled after  1.6123603833mins
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
>  (290578 ms)
> 
> Sample output with 51027:
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
> Using 1 agents and 3000 frameworks
> Added 3000 frameworks in 39817us
> Added 1 agents in 3.2286054167mins
> allocator settled after  25.525654secs
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
>  (220137 ms)
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

Re: Review Request 51027: Track allocation candidates to bound allocator.

[Jacob Janco]

> On Aug. 18, 2016, 12:38 a.m., Guangya Liu wrote:
> > Can you please also clarify some difference with 
> > https://reviews.apache.org/r/41658/, as here the work is carry from 
> > https://reviews.apache.org/r/41658/ , I think that this is more simple and 
> > clear compared with counter in https://reviews.apache.org/r/41658/ with a 
> > boolean here.
> 
> Jiang Yan Xu wrote:
> This is exactly a rework based on comments in 
> https://reviews.apache.org/r/41658/.
> 
> Guangya Liu wrote:
> Yes, but as the `Description` part mentioned 
> https://reviews.apache.org/r/41658/ , so here it would be great to list some 
> difference with https://reviews.apache.org/r/41658/ , and the most important 
> thing is with the `boolean` value here, we can avoid some no-op allocate 
> operations.

Added.


- Jacob


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51027/#review146058
---


On Aug. 23, 2016, 8:32 a.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51027/
> ---
> 
> (Updated Aug. 23, 2016, 8:32 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, James Peach, Klaus Ma, and Jiang 
> Yan Xu.
> 
> 
> Bugs: MESOS-3157
> https://issues.apache.org/jira/browse/MESOS-3157
> 
> 
> 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.
> - Batched allocations are handled synchronously.
> 
> - Carrying over work from https://reviews.apache.org/r/41658/ and added the 
> previous reviewers
> - Specifically, this patch introduces the boolean flag pendingAllocation, 
> which when set on event 
>   triggered allocations, will prevent additional no-op allocations: the flag 
> is cleared when 
>   the enqueued allocation is processed, subsequent event triggered 
> allocations will update a set
>   of allocation candidates rather than dispatching an additional allocate().
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> bdbc6d3b5b959990538f4e3b7b1a3b031d9aea05 
>   src/master/allocator/mesos/hierarchical.cpp 
> 234ef98529964a0b6d3f132426a6c8ccbb1263ee 
> 
> Diff: https://reviews.apache.org/r/51027/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> note: check without filters depends on https://reviews.apache.org/r/51028
> 
> With new benchmark https://reviews.apache.org/r/49617: 
> Sample output without 51027:
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
> Using 1 agents and 3000 frameworks
> Added 3000 frameworks in 57251us
> Added 1 agents in 3.2134535333mins
> allocator settled after  1.6123603833mins
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
>  (290578 ms)
> 
> Sample output with 51027:
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
> Using 1 agents and 3000 frameworks
> Added 3000 frameworks in 39817us
> Added 1 agents in 3.2286054167mins
> allocator settled after  25.525654secs
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
>  (220137 ms)
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

Re: Review Request 51027: Track allocation candidates to bound allocator.

[Jacob Janco]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51027/
---

(Updated Aug. 23, 2016, 8:32 a.m.)


Review request for mesos, Benjamin Mahler, James Peach, Klaus Ma, and Jiang Yan 
Xu.


Changes
---

Adding back reviewers and dependencies.


Bugs: MESOS-3157
https://issues.apache.org/jira/browse/MESOS-3157


Repository: mesos


Description (updated)
---

- 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.
- Batched allocations are handled synchronously.

- Carrying over work from https://reviews.apache.org/r/41658/ and added the 
previous reviewers
- Specifically, this patch introduces the boolean flag pendingAllocation, which 
when set on event 
  triggered allocations, will prevent additional no-op allocations: the flag is 
cleared when 
  the enqueued allocation is processed, subsequent event triggered allocations 
will update a set
  of allocation candidates rather than dispatching an additional allocate().


Diffs
-

  src/master/allocator/mesos/hierarchical.hpp 
bdbc6d3b5b959990538f4e3b7b1a3b031d9aea05 
  src/master/allocator/mesos/hierarchical.cpp 
234ef98529964a0b6d3f132426a6c8ccbb1263ee 

Diff: https://reviews.apache.org/r/51027/diff/


Testing
---

make check

note: check without filters depends on https://reviews.apache.org/r/51028

With new benchmark https://reviews.apache.org/r/49617: 
Sample output without 51027:
[ RUN  ] 
SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
Using 1 agents and 3000 frameworks
Added 3000 frameworks in 57251us
Added 1 agents in 3.2134535333mins
allocator settled after  1.6123603833mins
[   OK ] 
SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
 (290578 ms)

Sample output with 51027:
[ RUN  ] 
SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
Using 1 agents and 3000 frameworks
Added 3000 frameworks in 39817us
Added 1 agents in 3.2286054167mins
allocator settled after  25.525654secs
[   OK ] 
SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
 (220137 ms)


Thanks,

Jacob Janco

Re: Review Request 51027: Track allocation candidates to bound allocator.

[Jacob Janco]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51027/
---

(Updated Aug. 23, 2016, 8:24 a.m.)


Review request for .


Bugs: MESOS-3157
https://issues.apache.org/jira/browse/MESOS-3157


Repository: mesos


Description (updated)
---

- 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.
- Batched allocations are handled synchronously.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.hpp 
bdbc6d3b5b959990538f4e3b7b1a3b031d9aea05 
  src/master/allocator/mesos/hierarchical.cpp 
234ef98529964a0b6d3f132426a6c8ccbb1263ee 

Diff: https://reviews.apache.org/r/51027/diff/


Testing
---

make check

note: check without filters depends on https://reviews.apache.org/r/51028

With new benchmark https://reviews.apache.org/r/49617: 
Sample output without 51027:
[ RUN  ] 
SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
Using 1 agents and 3000 frameworks
Added 3000 frameworks in 57251us
Added 1 agents in 3.2134535333mins
allocator settled after  1.6123603833mins
[   OK ] 
SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
 (290578 ms)

Sample output with 51027:
[ RUN  ] 
SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
Using 1 agents and 3000 frameworks
Added 3000 frameworks in 39817us
Added 1 agents in 3.2286054167mins
allocator settled after  25.525654secs
[   OK ] 
SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
 (220137 ms)


Thanks,

Jacob Janco

Re: Review Request 51027: Track allocation candidates to bound allocator.

[Jacob Janco]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51027/
---

(Updated Aug. 23, 2016, 8:24 a.m.)


Review request for .


Bugs: MESOS-3157
https://issues.apache.org/jira/browse/MESOS-3157


Repository: mesos


Description (updated)
---

- 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.
- Batched allocations are handled synchronously.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.hpp 
bdbc6d3b5b959990538f4e3b7b1a3b031d9aea05 
  src/master/allocator/mesos/hierarchical.cpp 
234ef98529964a0b6d3f132426a6c8ccbb1263ee 

Diff: https://reviews.apache.org/r/51027/diff/


Testing
---

make check

note: check without filters depends on https://reviews.apache.org/r/51028

With new benchmark https://reviews.apache.org/r/49617: 
Sample output without 51027:
[ RUN  ] 
SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
Using 1 agents and 3000 frameworks
Added 3000 frameworks in 57251us
Added 1 agents in 3.2134535333mins
allocator settled after  1.6123603833mins
[   OK ] 
SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
 (290578 ms)

Sample output with 51027:
[ RUN  ] 
SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
Using 1 agents and 3000 frameworks
Added 3000 frameworks in 39817us
Added 1 agents in 3.2286054167mins
allocator settled after  25.525654secs
[   OK ] 
SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
 (220137 ms)


Thanks,

Jacob Janco

Re: Review Request 51027: Track allocation candidates to bound allocator.

[Guangya Liu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51027/#review146480
---



This can also help https://issues.apache.org/jira/browse/MESOS-3078 , we can 
put similar logic in `addSlave` to `recoverResources` so as to update the 
allocation candidate or allocate directly.

- Guangya Liu


On 八月 17, 2016, 6:12 p.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51027/
> ---
> 
> (Updated 八月 17, 2016, 6:12 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, James Peach, Klaus Ma, and Jiang 
> Yan Xu.
> 
> 
> Bugs: MESOS-3157
> https://issues.apache.org/jira/browse/MESOS-3157
> 
> 
> 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.
>   
>   - carrying over work from https://reviews.apache.org/r/41658/ and added the 
> previous reviewers
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> bdbc6d3b5b959990538f4e3b7b1a3b031d9aea05 
>   src/master/allocator/mesos/hierarchical.cpp 
> 234ef98529964a0b6d3f132426a6c8ccbb1263ee 
> 
> Diff: https://reviews.apache.org/r/51027/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> note: check without filters depends on https://reviews.apache.org/r/51028
> 
> With new benchmark https://reviews.apache.org/r/49617: 
> Sample output without 51027:
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
> Using 1 agents and 3000 frameworks
> Added 3000 frameworks in 57251us
> Added 1 agents in 3.2134535333mins
> allocator settled after  1.6123603833mins
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
>  (290578 ms)
> 
> Sample output with 51027:
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
> Using 1 agents and 3000 frameworks
> Added 3000 frameworks in 39817us
> Added 1 agents in 3.2286054167mins
> allocator settled after  25.525654secs
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
>  (220137 ms)
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

Re: Review Request 51027: Track allocation candidates to bound allocator.

[Guangya Liu]

> On 八月 18, 2016, 12:38 a.m., Guangya Liu wrote:
> > Can you please also clarify some difference with 
> > https://reviews.apache.org/r/41658/, as here the work is carry from 
> > https://reviews.apache.org/r/41658/ , I think that this is more simple and 
> > clear compared with counter in https://reviews.apache.org/r/41658/ with a 
> > boolean here.
> 
> Jiang Yan Xu wrote:
> This is exactly a rework based on comments in 
> https://reviews.apache.org/r/41658/.

Yes, but as the `Description` part mentioned 
https://reviews.apache.org/r/41658/ , so here it would be great to list some 
difference with https://reviews.apache.org/r/41658/ , and the most important 
thing is with the `boolean` value here, we can avoid some no-op allocate 
operations.


- Guangya


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51027/#review146058
---


On 八月 17, 2016, 6:12 p.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51027/
> ---
> 
> (Updated 八月 17, 2016, 6:12 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, James Peach, Klaus Ma, and Jiang 
> Yan Xu.
> 
> 
> Bugs: MESOS-3157
> https://issues.apache.org/jira/browse/MESOS-3157
> 
> 
> 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.
>   
>   - carrying over work from https://reviews.apache.org/r/41658/ and added the 
> previous reviewers
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> bdbc6d3b5b959990538f4e3b7b1a3b031d9aea05 
>   src/master/allocator/mesos/hierarchical.cpp 
> 234ef98529964a0b6d3f132426a6c8ccbb1263ee 
> 
> Diff: https://reviews.apache.org/r/51027/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> note: check without filters depends on https://reviews.apache.org/r/51028
> 
> With new benchmark https://reviews.apache.org/r/49617: 
> Sample output without 51027:
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
> Using 1 agents and 3000 frameworks
> Added 3000 frameworks in 57251us
> Added 1 agents in 3.2134535333mins
> allocator settled after  1.6123603833mins
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
>  (290578 ms)
> 
> Sample output with 51027:
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
> Using 1 agents and 3000 frameworks
> Added 3000 frameworks in 39817us
> Added 1 agents in 3.2286054167mins
> allocator settled after  25.525654secs
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
>  (220137 ms)
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

Re: Review Request 51027: Track allocation candidates to bound allocator.

[Jiang Yan Xu]

> On Aug. 17, 2016, 5:36 p.m., Guangya Liu wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 29
> > 
> >
> > move this right before `#include `

Also move `#include ` with it.


- Jiang Yan


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51027/#review146040
---


On Aug. 17, 2016, 11:12 a.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51027/
> ---
> 
> (Updated Aug. 17, 2016, 11:12 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, James Peach, Klaus Ma, and Jiang 
> Yan Xu.
> 
> 
> Bugs: MESOS-3157
> https://issues.apache.org/jira/browse/MESOS-3157
> 
> 
> 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.
>   
>   - carrying over work from https://reviews.apache.org/r/41658/ and added the 
> previous reviewers
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> bdbc6d3b5b959990538f4e3b7b1a3b031d9aea05 
>   src/master/allocator/mesos/hierarchical.cpp 
> 234ef98529964a0b6d3f132426a6c8ccbb1263ee 
> 
> Diff: https://reviews.apache.org/r/51027/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> note: check without filters depends on https://reviews.apache.org/r/51028
> 
> With new benchmark https://reviews.apache.org/r/49617: 
> Sample output without 51027:
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
> Using 1 agents and 3000 frameworks
> Added 3000 frameworks in 57251us
> Added 1 agents in 3.2134535333mins
> allocator settled after  1.6123603833mins
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
>  (290578 ms)
> 
> Sample output with 51027:
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
> Using 1 agents and 3000 frameworks
> Added 3000 frameworks in 39817us
> Added 1 agents in 3.2286054167mins
> allocator settled after  25.525654secs
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
>  (220137 ms)
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

Re: Review Request 51027: Track allocation candidates to bound allocator.

[Jiang Yan Xu]

> On Aug. 17, 2016, 5:38 p.m., Guangya Liu wrote:
> > Can you please also clarify some difference with 
> > https://reviews.apache.org/r/41658/, as here the work is carry from 
> > https://reviews.apache.org/r/41658/ , I think that this is more simple and 
> > clear compared with counter in https://reviews.apache.org/r/41658/ with a 
> > boolean here.

This is exactly a rework based on comments in 
https://reviews.apache.org/r/41658/.


- Jiang Yan


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51027/#review146058
---


On Aug. 17, 2016, 11:12 a.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51027/
> ---
> 
> (Updated Aug. 17, 2016, 11:12 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, James Peach, Klaus Ma, and Jiang 
> Yan Xu.
> 
> 
> Bugs: MESOS-3157
> https://issues.apache.org/jira/browse/MESOS-3157
> 
> 
> 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.
>   
>   - carrying over work from https://reviews.apache.org/r/41658/ and added the 
> previous reviewers
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> bdbc6d3b5b959990538f4e3b7b1a3b031d9aea05 
>   src/master/allocator/mesos/hierarchical.cpp 
> 234ef98529964a0b6d3f132426a6c8ccbb1263ee 
> 
> Diff: https://reviews.apache.org/r/51027/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> note: check without filters depends on https://reviews.apache.org/r/51028
> 
> With new benchmark https://reviews.apache.org/r/49617: 
> Sample output without 51027:
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
> Using 1 agents and 3000 frameworks
> Added 3000 frameworks in 57251us
> Added 1 agents in 3.2134535333mins
> allocator settled after  1.6123603833mins
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
>  (290578 ms)
> 
> Sample output with 51027:
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
> Using 1 agents and 3000 frameworks
> Added 3000 frameworks in 39817us
> Added 1 agents in 3.2286054167mins
> allocator settled after  25.525654secs
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
>  (220137 ms)
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

Re: Review Request 51027: Track allocation candidates to bound allocator.

[Jiang Yan Xu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51027/#review146094
---




src/master/allocator/mesos/hierarchical.hpp (line 216)


Change the comment to:

```
// Allocate resources from the allocation candidates.
```



src/master/allocator/mesos/hierarchical.hpp (lines 219 - 220)


Kill this. We don't define this method anymore.



src/master/allocator/mesos/hierarchical.hpp (lines 222 - 223)


We need to change this per the comments below.



src/master/allocator/mesos/hierarchical.hpp (lines 382 - 383)


We should add comments for these members.



src/master/allocator/mesos/hierarchical.cpp (lines 782 - 786)


Here it's appropriate, particularly given the comment above, that we run 
allocation synchronously.

Add a comment similar to the one in `setQuota` on why we do it 
synchronously.



src/master/allocator/mesos/hierarchical.cpp (line 1107)


s/explicitly/synchronously/



src/master/allocator/mesos/hierarchical.cpp (lines 1109 - 1113)


Here it's appropriate, particularly given the comment above, that we run 
allocation synchronously.



src/master/allocator/mesos/hierarchical.cpp (line 1136)


s/explicitly/synchronously/



src/master/allocator/mesos/hierarchical.cpp (lines 1138 - 1142)


Ditto.



src/master/allocator/mesos/hierarchical.cpp (lines 1177 - 1181)


Ditto.



src/master/allocator/mesos/hierarchical.cpp (line 1209)


We need some comments here.

```
We run the allocation synchronously here instead of dispatching it so we 
don't delay a batched allocation if the allocator is backed up.

TODO: This still does not guarantee that at least one allocation is run per 
batch interval. Consider having the allocator run allocations synchronously 
when handling events if no allocation is run for the past batch interval. 
```



src/master/allocator/mesos/hierarchical.cpp (line 1214)






src/master/allocator/mesos/hierarchical.cpp (line 1227)


Here we can just rename the method we call as: `_allocate()`. Note that now 
that we keep track of a member `allocationCandidates` we don't have to pass 
them around. If we don't pass them around, we can't use the argument as what 
distinguishes the overloaded `allocate()` methods.

I think it's pretty natural to have `void _allocate();` as a private helper.



src/master/allocator/mesos/hierarchical.cpp (line 1582)


Similarly here, we don't need the argument in `deallocate()`.


- Jiang Yan Xu


On Aug. 17, 2016, 11:12 a.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51027/
> ---
> 
> (Updated Aug. 17, 2016, 11:12 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, James Peach, Klaus Ma, and Jiang 
> Yan Xu.
> 
> 
> Bugs: MESOS-3157
> https://issues.apache.org/jira/browse/MESOS-3157
> 
> 
> 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.
>   
>   - carrying over work from https://reviews.apache.org/r/41658/ and added the 
> previous reviewers
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> bdbc6d3b5b959990538f4e3b7b1a3b031d9aea05 
>   src/master/allocator/mesos/hierarchical.cpp 
> 234ef98529964a0b6d3f132426a6c8ccbb1263ee 
> 
> Diff: https://reviews.apache.org/r/51027/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> note: check without filters depends on https://reviews.apache.org/r/51028
> 
> With new benchmark https://reviews.apache.org/r/49617: 
> Sample output without 51027:
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
> Using 1 agents and 3000 frameworks
> Added 3000 frameworks in 57251us
> Added 1 agents in 3.2134535333mins
> allocator settled after  1.6123603833mins
> [   

Re: Review Request 51027: Track allocation candidates to bound allocator.

[Guangya Liu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51027/#review146058
---



Can you please also clarify some difference with 
https://reviews.apache.org/r/41658/, as here the work is carry from 
https://reviews.apache.org/r/41658/ , I think that this is more simple and 
clear compared with counter in https://reviews.apache.org/r/41658/ with a 
boolean here.

- Guangya Liu


On 八月 17, 2016, 6:12 p.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51027/
> ---
> 
> (Updated 八月 17, 2016, 6:12 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, James Peach, Klaus Ma, and Jiang 
> Yan Xu.
> 
> 
> Bugs: MESOS-3157
> https://issues.apache.org/jira/browse/MESOS-3157
> 
> 
> 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.
>   
>   - carrying over work from https://reviews.apache.org/r/41658/ and added the 
> previous reviewers
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> bdbc6d3b5b959990538f4e3b7b1a3b031d9aea05 
>   src/master/allocator/mesos/hierarchical.cpp 
> 234ef98529964a0b6d3f132426a6c8ccbb1263ee 
> 
> Diff: https://reviews.apache.org/r/51027/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> note: check without filters depends on https://reviews.apache.org/r/51028
> 
> With new benchmark https://reviews.apache.org/r/49617: 
> Sample output without 51027:
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
> Using 1 agents and 3000 frameworks
> Added 3000 frameworks in 57251us
> Added 1 agents in 3.2134535333mins
> allocator settled after  1.6123603833mins
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
>  (290578 ms)
> 
> Sample output with 51027:
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
> Using 1 agents and 3000 frameworks
> Added 3000 frameworks in 39817us
> Added 1 agents in 3.2286054167mins
> allocator settled after  25.525654secs
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
>  (220137 ms)
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

Re: Review Request 51027: Track allocation candidates to bound allocator.

[Guangya Liu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51027/#review146040
---




src/master/allocator/mesos/hierarchical.hpp (lines 381 - 382)


new line here



src/master/allocator/mesos/hierarchical.hpp (lines 382 - 383)


new line here



src/master/allocator/mesos/hierarchical.cpp (line 29)


move this right before `#include `



src/master/allocator/mesos/hierarchical.cpp (line 1232)


Keep align with above `<<`.

```
VLOG(1) << "Performed allocation for " << allocationCandidates.size()
<< " agents in " << stopwatch.elapsed();
```


- Guangya Liu


On 八月 17, 2016, 6:12 p.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51027/
> ---
> 
> (Updated 八月 17, 2016, 6:12 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, James Peach, Klaus Ma, and Jiang 
> Yan Xu.
> 
> 
> Bugs: MESOS-3157
> https://issues.apache.org/jira/browse/MESOS-3157
> 
> 
> 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.
>   
>   - carrying over work from https://reviews.apache.org/r/41658/ and added the 
> previous reviewers
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> bdbc6d3b5b959990538f4e3b7b1a3b031d9aea05 
>   src/master/allocator/mesos/hierarchical.cpp 
> 234ef98529964a0b6d3f132426a6c8ccbb1263ee 
> 
> Diff: https://reviews.apache.org/r/51027/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> note: check without filters depends on https://reviews.apache.org/r/51028
> 
> With new benchmark https://reviews.apache.org/r/49617: 
> Sample output without 51027:
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
> Using 1 agents and 3000 frameworks
> Added 3000 frameworks in 57251us
> Added 1 agents in 3.2134535333mins
> allocator settled after  1.6123603833mins
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
>  (290578 ms)
> 
> Sample output with 51027:
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
> Using 1 agents and 3000 frameworks
> Added 3000 frameworks in 39817us
> Added 1 agents in 3.2286054167mins
> allocator settled after  25.525654secs
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
>  (220137 ms)
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

Re: Review Request 51027: Track allocation candidates to bound allocator.

[Jacob Janco]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51027/
---

(Updated Aug. 17, 2016, 6:12 p.m.)


Review request for mesos, Benjamin Mahler, James Peach, Klaus Ma, and Jiang Yan 
Xu.


Bugs: MESOS-3157
https://issues.apache.org/jira/browse/MESOS-3157


Repository: mesos


Description (updated)
---

- 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.
  
  - carrying over work from https://reviews.apache.org/r/41658/ and added the 
previous reviewers


Diffs
-

  src/master/allocator/mesos/hierarchical.hpp 
bdbc6d3b5b959990538f4e3b7b1a3b031d9aea05 
  src/master/allocator/mesos/hierarchical.cpp 
234ef98529964a0b6d3f132426a6c8ccbb1263ee 

Diff: https://reviews.apache.org/r/51027/diff/


Testing
---

make check

note: check without filters depends on https://reviews.apache.org/r/51028

With new benchmark https://reviews.apache.org/r/49617: 
Sample output without 51027:
[ RUN  ] 
SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
Using 1 agents and 3000 frameworks
Added 3000 frameworks in 57251us
Added 1 agents in 3.2134535333mins
allocator settled after  1.6123603833mins
[   OK ] 
SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
 (290578 ms)

Sample output with 51027:
[ RUN  ] 
SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
Using 1 agents and 3000 frameworks
Added 3000 frameworks in 39817us
Added 1 agents in 3.2286054167mins
allocator settled after  25.525654secs
[   OK ] 
SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
 (220137 ms)


Thanks,

Jacob Janco

Re: Review Request 51027: Track allocation candidates to bound allocator.

[Mesos ReviewBot]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51027/#review145980
---



Bad review!

Reviews applied: []

Error:
No reviewers specified. Please find a reviewer by asking on JIRA or the mailing 
list.

- Mesos ReviewBot


On Aug. 17, 2016, 3:51 a.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51027/
> ---
> 
> (Updated Aug. 17, 2016, 3:51 a.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-3157
> https://issues.apache.org/jira/browse/MESOS-3157
> 
> 
> 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 
> bdbc6d3b5b959990538f4e3b7b1a3b031d9aea05 
>   src/master/allocator/mesos/hierarchical.cpp 
> 234ef98529964a0b6d3f132426a6c8ccbb1263ee 
> 
> Diff: https://reviews.apache.org/r/51027/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> note: check without filters depends on https://reviews.apache.org/r/51028
> 
> With new benchmark https://reviews.apache.org/r/49617: 
> Sample output without 51027:
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
> Using 1 agents and 3000 frameworks
> Added 3000 frameworks in 57251us
> Added 1 agents in 3.2134535333mins
> allocator settled after  1.6123603833mins
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
>  (290578 ms)
> 
> Sample output with 51027:
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
> Using 1 agents and 3000 frameworks
> Added 3000 frameworks in 39817us
> Added 1 agents in 3.2286054167mins
> allocator settled after  25.525654secs
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
>  (220137 ms)
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

Re: Review Request 51027: Track allocation candidates to bound allocator.

[Jacob Janco]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51027/
---

(Updated Aug. 17, 2016, 3:51 a.m.)


Review request for mesos.


Bugs: MESOS-3157
https://issues.apache.org/jira/browse/MESOS-3157


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 
bdbc6d3b5b959990538f4e3b7b1a3b031d9aea05 
  src/master/allocator/mesos/hierarchical.cpp 
234ef98529964a0b6d3f132426a6c8ccbb1263ee 

Diff: https://reviews.apache.org/r/51027/diff/


Testing (updated)
---

make check

note: check without filters depends on https://reviews.apache.org/r/51028

With new benchmark https://reviews.apache.org/r/49617: 
Sample output without 51027:
[ RUN  ] 
SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
Using 1 agents and 3000 frameworks
Added 3000 frameworks in 57251us
Added 1 agents in 3.2134535333mins
allocator settled after  1.6123603833mins
[   OK ] 
SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
 (290578 ms)

Sample output with 51027:
[ RUN  ] 
SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
Using 1 agents and 3000 frameworks
Added 3000 frameworks in 39817us
Added 1 agents in 3.2286054167mins
allocator settled after  25.525654secs
[   OK ] 
SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
 (220137 ms)


Thanks,

Jacob Janco

Re: Review Request 51027: Track allocation candidates to bound allocator.

[Jacob Janco]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51027/
---

(Updated Aug. 17, 2016, 3:46 a.m.)


Review request for mesos.


Bugs: MESOS-3157
https://issues.apache.org/jira/browse/MESOS-3157


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 
bdbc6d3b5b959990538f4e3b7b1a3b031d9aea05 
  src/master/allocator/mesos/hierarchical.cpp 
234ef98529964a0b6d3f132426a6c8ccbb1263ee 

Diff: https://reviews.apache.org/r/51027/diff/


Testing (updated)
---

make check

note: check without filters depends on https://reviews.apache.org/r/51028


Thanks,

Jacob Janco