mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Mahler <bmah...@apache.org>
Subject Re: Review Request 67826: Made `Slave::getAvailable()` return all shared resources.
Date Mon, 23 Jul 2018 23:09:29 GMT

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


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