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 69890: Added test for per-framework, per-role minimal allocatable resources.
Date Tue, 05 Feb 2019 19:03:41 GMT

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



Looks good! Just some minor comments below


src/tests/hierarchical_allocator_tests.cpp
Lines 2287 (patched)
<https://reviews.apache.org/r/69890/#comment298386>

    any reason not to stick to using "minimum" for these? ditto below
    
    is "finite" needed here?



src/tests/hierarchical_allocator_tests.cpp
Lines 2287-2288 (patched)
<https://reviews.apache.org/r/69890/#comment298387>

    suggestion:
    
    "resources, it correctly overrides the global minimum. We check this by ensuring it gets
an offer when the global minimum is not satisfied but the framework's role specific mininium
is"



src/tests/hierarchical_allocator_tests.cpp
Lines 2296 (patched)
<https://reviews.apache.org/r/69890/#comment298385>

    Can we pass a master::Flags object into initialize that has an explicit min allocatable
resources flag set? (It seems it already is set up to take this flag)
    
    Right now this test seems to be making a pretty strong assumption that MIN_CPUS is what's
being used?
    
    Ditto for the other tests below



src/tests/hierarchical_allocator_tests.cpp
Lines 2345-2346 (patched)
<https://reviews.apache.org/r/69890/#comment298391>

    seems like this should describe that "a single empty quantity" is used?



src/tests/hierarchical_allocator_tests.cpp
Lines 2352-2353 (patched)
<https://reviews.apache.org/r/69890/#comment298388>

    Do we actually need these two lines? It seems like having a "set" but empty `min_allocatable_resources`
should be enough? (i.e. without having to add a single array item within its `quantities`)



src/tests/hierarchical_allocator_tests.cpp
Lines 2357 (patched)
<https://reviews.apache.org/r/69890/#comment298389>

    Ditto earlier comment, it's probably not helpful for  the reader to see a mixture of "minimal"
and "minimum" used?



src/tests/hierarchical_allocator_tests.cpp
Lines 2378-2381 (patched)
<https://reviews.apache.org/r/69890/#comment298390>

    This seems like the "EmptyMinAllocatable" case and the one above seems like the "EmptyQuantityMinAllocatable"
case?


- Benjamin Mahler


On Feb. 4, 2019, 10:05 p.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69890/
> -----------------------------------------------------------
> 
> (Updated Feb. 4, 2019, 10:05 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Meng Zhu.
> 
> 
> Bugs: MESOS-9523
>     https://issues.apache.org/jira/browse/MESOS-9523
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This patch adds tests for the behavior of per-framework, per-role
> minimal allocatable resources. We validate the behavior of framework
> minimal allocatable resources below the globally configured limits and
> the behavior of empty filters or minimal resource quantities set up for
> a framework.
> 
> 
> Diffs
> -----
> 
>   src/tests/hierarchical_allocator_tests.cpp cc88afbad1b4e6bf707cb13b50c964aa01f9a3ee

> 
> 
> Diff: https://reviews.apache.org/r/69890/diff/1/
> 
> 
> Testing
> -------
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>


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