mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Bannier <benjamin.bann...@mesosphere.io>
Subject Re: Review Request 69942: Added a test for MESOS-9554.
Date Tue, 12 Feb 2019 18:34:37 GMT

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


Fix it, then Ship it!





src/tests/hierarchical_allocator_tests.cpp
Lines 2924-2929 (patched)
<https://reviews.apache.org/r/69942/#comment298655>

    This test actually is more than a regression test for MESOS-9554 as it validates to some
extent that framework allocation decisions are independent (but for possibly decreasing the
available resource pool).
    
    Maybe rephrase along the lines of
    ```
    This test validates with one case that framework allocation decisions are independent
(but for possibly decreasing the available resource pool). In particular, we validate that
the decision not to allocate resources of an agent to a particular framework does not influence
to allocate the same resources to another agent. In this test we make agent resources not
allocatable to a framework by refining the resource reservations while the framework which
should be allocated these resources based on DRF share is not capable of receiving them (it
lacks the `RESERVATION_REFINEMENT` capability).
    
    This is a regression test for MESOS-9554.
    ```
    
    We could also rename the test, e.g., to `HierarchicalAllocatorTest.AllocationIndependent`
(doesn't read to well, either). This tests models a whole class of scenarios we currently
don't validate.



src/tests/hierarchical_allocator_tests.cpp
Lines 2970 (patched)
<https://reviews.apache.org/r/69942/#comment298654>

    Let's `ASSERT_EQ` here as the test is pointless without this being true.



src/tests/hierarchical_allocator_tests.cpp
Lines 2984-2990 (patched)
<https://reviews.apache.org/r/69942/#comment298656>

    I don't think using a single statement here reads much worse,
    ```
    Resources resources =?
      CHECK_NOTERROR(Resources::parse("cpus:1;mem:1;disk:1"))?
        .pushReservation(createDynamicReservationInfo("parent"))?
        .pushReservation(createDynamicReservationInfo("parent/child"));?
    ```


- Benjamin Bannier


On Feb. 11, 2019, 7:21 p.m., Benjamin Mahler wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69942/
> -----------------------------------------------------------
> 
> (Updated Feb. 11, 2019, 7:21 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Meng Zhu.
> 
> 
> Bugs: MESOS-9554
>     https://issues.apache.org/jira/browse/MESOS-9554
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This test ensures that the framework allocation loop does not
> incorrectly break in the case that a framework is incapable
> of receiving the resources on an agent. In this case, the loop
> should continue as other frameworks may be capable of receiving
> the resources.
> 
> 
> Diffs
> -----
> 
>   src/tests/hierarchical_allocator_tests.cpp cc88afbad1b4e6bf707cb13b50c964aa01f9a3ee

> 
> 
> Diff: https://reviews.apache.org/r/69942/diff/1/
> 
> 
> Testing
> -------
> 
> Ensured it fails on master, succeeds after fix to MESOS-9554.
> 
> Ran in repetition.
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>


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