mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Meng Zhu <m...@mesosphere.io>
Subject Re: Review Request 67826: Made `Slave::getAvailable()` return all shared resources.
Date Tue, 24 Jul 2018 19:50:45 GMT


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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message