----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71460/#review217673 -----------------------------------------------------------
src/master/allocator/mesos/hierarchical.cpp Lines 416-422 (patched) <https://reviews.apache.org/r/71460/#comment304967> Hmm... when `rolesConsumedQuota` was calculated using `Sorter::allocation()`, quantities of shared resources belonging to the same slave were accounted for only once, but quantitoes of shared resources belonging to different slaves were simply added. Does this still hold? If yes, then how exactly is `RoleTree` now capable of this without knowing `SlaveID`s? If no - then does quota still work correctly for shared resources? (If it doesn't, this makes me wonder about our test coverage as well). src/master/allocator/mesos/hierarchical.cpp Lines 417-420 (patched) <https://reviews.apache.org/r/71460/#comment304965> Shouldn't this be `CHECK_CONTAINS(roles_, role)` and `roles_.at(role)` instead? If, due to some bug, this method is called for a non-existing role, this code will add an empty role with some `offeredOrAllocated_`, which violates the contract you are introducting and might crash `tryRemove`. I'd rather prefer to fail here - things will be simpler to debug. src/master/allocator/mesos/hierarchical.cpp Lines 440 (patched) <https://reviews.apache.org/r/71460/#comment304966> Untrack? - Andrei Sekretenko On Sept. 9, 2019, 8:58 p.m., Meng Zhu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71460/ > ----------------------------------------------------------- > > (Updated Sept. 9, 2019, 8:58 p.m.) > > > Review request for mesos, Andrei Sekretenko and Benjamin Mahler. > > > Repository: mesos > > > Description > ------- > > This helpers simplify the quota tracking logic and also paves > the way to reduce duplicated states in the sorter. > > Small performance degradation when making allocations due to > duplicated map construction in `(un)trackAllocatedResources`. > This will be removed once embeded the sorter in the role tree. > > Benchmark `LargeAndSmallQuota/2`: > > Master: > > Added 3000 agents in 80.648188ms > Added 3000 frameworks in 19.7006984secs > Benchmark setup: 3000 agents, 3000 roles, 3000 frameworks, > with drf sorter > Made 3500 allocations in 16.044274434secs > Made 0 allocation in 14.476429451secs > > Master + this patch: > Added 3000 agents in 80.110817ms > Added 3000 frameworks in 17.25974094secs > Benchmark setup: 3000 agents, 3000 roles, 3000 frameworks, > with drf sorter > Made 3500 allocations in 16.91971379secs > Made 0 allocation in 13.784476154secs > > > Diffs > ----- > > src/master/allocator/mesos/hierarchical.hpp > 48ba399e12129b3348b5c43b793f3428acf26d4e > src/master/allocator/mesos/hierarchical.cpp > 187de173ee2f563e2ff0f7e5c96695dd94a73970 > > > Diff: https://reviews.apache.org/r/71460/diff/1/ > > > Testing > ------- > > make check > Benchmark result in the description > > > Thanks, > > Meng Zhu > >
