> On July 23, 2018, 4:09 p.m., Benjamin Mahler wrote:
> > 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`?
The existing implementation:
```
Resources available = slave.getAvailable().nonShared();
if (framework.capabilities.sharedResources) {
available += slave.getTotal().shared();
...
}
```
does not depend on how much shared resources `getAvailable()` will return -- it
calls `nonShared()` anyway. I think there is no assumption being broken by this
patch and thus no need to tidy the logic in the allocation loop.
Added a comment about why all shared resources are always included.
> On July 23, 2018, 4:09 p.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.hpp
> > Lines 474-476 (patched)
> > <https://reviews.apache.org/r/67826/diff/3/?file=2061976#file2061976line474>
> >
> > 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?
See my comment above.
> On July 23, 2018, 4:09 p.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.hpp
> > Lines 509-511 (patched)
> > <https://reviews.apache.org/r/67826/diff/3/?file=2061976#file2061976line509>
> >
> > 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?
Done.
- Meng
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67826/#review206364
-----------------------------------------------------------
On July 24, 2018, 12:50 p.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67826/
> -----------------------------------------------------------
>
> (Updated July 24, 2018, 12:50 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
> c1a6789f1808a57dd94ede7bbd2636031f136ea3
>
>
> Diff: https://reviews.apache.org/r/67826/diff/4/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Meng Zhu
>
>
