> On Jan. 15, 2016, 6:21 p.m., Joseph Wu wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 529-535
> > <https://reviews.apache.org/r/41847/diff/4/?file=1195636#file1195636line529>
> >
> > What if you did this?
> > ```
> > slaves[slaveId]total = slaves[slaveId].total.nonUsageSlack() +
> > oversubscribed;
> > ```
> >
> > Where `nonUsageSlack` is a helper that filters out usage slack. (You
> > could just use a Resource filter directly here.)
> >
> > ---
> >
> > This would have fewer operations too (1 filter operation instead of 2).
>
> Guangya Liu wrote:
> I think that the current logic is that when
> enableReservationOversubscription is true, we only need to add the allocation
> slack to the total resources, there is no other change except this.
>
> The `allocationSlack()` also has only 1 filter `isAllocationSlack` ,
> comments?
>
> Joseph Wu wrote:
> I'm talking about the `.nonRevocable()` and the `allocationSlack()`
> filters.
>
> The current patch is a pretty roundabout way of writing this:
> ```
> slaves[slaveId].total = oversubscribed +
> slaves[slaveId].total.filter([](const Resource& resource) { return
> !isUsageSlack(resource); });
> ```
>
> ^ This makes it explicit that we're just resetting the `USAGE_SLACK`
> component and leaving everything else untouched.
>
> Guangya Liu wrote:
> My logic here is that for update `updateSlave`, do not touch other logic
> but only add the `allocationSlack` when `enableReservationOversubscription`
> is enabled, this may be cleaner? As I was only adding `allocationSlack` when
> `enableReservationOversubscription` is enabled.
>
> Joseph Wu wrote:
> The `enableReservationOversubscription` flag shouldn't even matter here.
> If you really want to be strict, you can add this:
> ```
> if (enableReservationOversubscription) {
> CHECK_EQ(Resources(), slaves[slaveId].total.allocationSlack());
> }
> ```
>
> Guangya Liu wrote:
> @Joseph, so the logic you proposed is as this?
>
> if (enableReservationOversubscription) {
> slaves[slaveId]total = slaves[slaveId].total.nonUsageSlack() +
> oversubscribed;
> } else {
> slaves[slaveId].total = total.nonRevocable() + oversubscribed;
> }
>
> Yes, the above logic can make allocator call only 1 filter. If this is
> your proposal, I will update the patch and move the `nonUsage` before this
> patch.
Not quite:
```
slaves[slaveId].total = slaves[slaveId].total.nonUsageSlack() + oversubscribed;
// Only add this if you really want to check the flag.
// But optimistic offers shouldn't even be considered in this method.
if (enableReservationOversubscription) {
CHECK_EQ(Resources(), slaves[slaveId].total.allocationSlack());
}
```
- Joseph
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41847/#review114832
-----------------------------------------------------------
On Jan. 13, 2016, 4:55 a.m., Guangya Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41847/
> -----------------------------------------------------------
>
> (Updated Jan. 13, 2016, 4:55 a.m.)
>
>
> Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van
> Remoortere, Joseph Wu, Klaus Ma, and Jian Qiu.
>
>
> Bugs: MESOS-4145
> https://issues.apache.org/jira/browse/MESOS-4145
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Updated allocation slack when slave was updated.
>
>
> Diffs
> -----
>
> src/master/allocator/mesos/hierarchical.cpp
> d541bfa3f4190865c65d35c9d1ffdb8a3f194056
> src/tests/hierarchical_allocator_tests.cpp
> e044f832c2c16e53e663c6ced5452649bb0dcb59
>
> Diff: https://reviews.apache.org/r/41847/diff/
>
>
> Testing
> -------
>
> make
> make check
> GLOG_v=2 ./bin/mesos-tests.sh --gtest_filter="HierarchicalAllocatorTest.*"
> --verbose
>
>
> Thanks,
>
> Guangya Liu
>
>
