mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alexander Rukletsov" <ruklet...@gmail.com>
Subject Re: Review Request 42289: Quota doesn't allocate resources on slave joining.
Date Tue, 19 Jan 2016 09:23:49 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42289/#review115132
-----------------------------------------------------------


Let's tweak some wording and testing and we are good to go!

I liked the initial summary more. IMO a patch should describe the solution, and not the problem.
It's quite opposite for JIRA tickets, hence I'm convinced patches and tickets should not share
the same title. How about "Calcuated 'remainingClusterResources' using all available agents."

I also think it won't hurt to extend the description and explain why we need this change.
How about
"Event-triggered allocations do not include all available agents. If we
calculate remaining resources in the cluster using the partial view,
we may overlook already laid away resources for quota'ed roles and lay
away more. Hence we may unnecessarily deprive non-quota'ed frameworks
of resources."

Changes touching allocator are vulnerable to races, especially in tests. Please extend the
testing (and mention this in the "Testing Done" section) with goodies like `GTEST_FILTER="*HierarchicalAllocatorTest*"
./bin/mesos-tests.sh --gtest_shuffle --gtest-repeat`.

Let's add a test for this change. Ideally the test should fail without the change and pass
it with the change. I think Neil has already provided the test in the ticket, could you please
include it?


src/master/allocator/mesos/hierarchical.cpp (lines 1227 - 1228)
<https://reviews.apache.org/r/42289/#comment175992>

    Let's add a note here that we iterate over all slaves and why. How about
    
    // NOTE: We use all active agents and not just those visible in the current `allocate()`
call so that frameworks in roles without quota are not unnecessarily deprived of resources.


- Alexander Rukletsov


On Jan. 19, 2016, 8:50 a.m., Klaus Ma wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42289/
> -----------------------------------------------------------
> 
> (Updated Jan. 19, 2016, 8:50 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Ben Mahler, Joris Van Remoortere, and
Neil Conway.
> 
> 
> Bugs: MESOS-4102
>     https://issues.apache.org/jira/browse/MESOS-4102
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Calcuated 'remainingClusterResources' by all activated slaves.
> 
> 
> Diffs
> -----
> 
>   src/master/allocator/mesos/hierarchical.cpp 48acde69b1a2f305b568a7e322a58708063dd30a

> 
> Diff: https://reviews.apache.org/r/42289/diff/
> 
> 
> Testing
> -------
> 
> make
> make check
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>


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