----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67826/#review206364 -----------------------------------------------------------
Can you clarify why this commit doesn't touch the shared resources logic in the allocation loop? https://github.com/apache/mesos/blob/1.6.0/src/master/allocator/mesos/hierarchical.cpp#L2070-L2079 Does this patch pass the tests without the next patch applied? Even so, it seems like this patch should tidy the shared resources logic up in the allocation loop now that the old assumption (doesn't show up in `getAvailable()`) isn't true anymore? Perhaps the following would be a bit simpler to follow, since the optimization seems to complicate things a bit: Patch 1: Move shared resources logic from the allocation loop into the agent struct. Patch 2: Optimize `Slave::updateAvailable` at the expense of `Slave` construction and `Slave::updateTotal`? src/master/allocator/mesos/hierarchical.hpp Line 464 (original), 471-476 (patched) <https://reviews.apache.org/r/67826/#comment289302> How about: ``` // Calling `nonShared()` currently copies the underlying resources // and is therefore rather expensive. We avoid it in the common // case that there are no shared resources. // // TODO(mzhu): Ideally there would be a single logical path here. // One solution is to have Resources be copy-on-write such that // `nonShared()` performs no copying and instead points to a // subset of the original `Resource` objects. ``` src/master/allocator/mesos/hierarchical.hpp Lines 474-476 (patched) <https://reviews.apache.org/r/67826/#comment289303> I was going to say this warrants a comment about there always being a copy of the shared resources available but I noticed its down below on the member variable. Per the comment at the top level of the review, shouldn't the comment/logic in the allocation loop be migrating here in this patch? src/master/allocator/mesos/hierarchical.hpp Lines 509-511 (patched) <https://reviews.apache.org/r/67826/#comment289301> s/traversals/copying/ ? Isn't the optimization also being done in the Slave constructor? Maybe we don't need to say exactly which functions the optimization is done? - Benjamin Mahler On July 20, 2018, 11:30 p.m., Meng Zhu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/67826/ > ----------------------------------------------------------- > > (Updated July 20, 2018, 11:30 p.m.) > > > Review request for mesos, Benjamin Mahler and Jiang Yan Xu. > > > Bugs: MESOS-9104 > https://issues.apache.org/jira/browse/MESOS-9104 > > > Repository: mesos > > > Description > ------- > > Currently, depending on already allocated resources, > `HierarchicalAllocatorProcess::Slave::getAvailable()` > may not contain all the shared resources. > Thus it does not accurately reflect what is available. > > Since shared resources are always allocatable, we should > include all shared resources in the agent available resources. > This would help remove the one off logic for removing and > adding back shared resources in the allocation loop. > > > Diffs > ----- > > src/master/allocator/mesos/hierarchical.hpp > 702e7c0aa84b4b672d82c759c25a28a77c78ad50 > > > Diff: https://reviews.apache.org/r/67826/diff/3/ > > > Testing > ------- > > make check > > > Thanks, > > Meng Zhu > >
