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 64493: Added tests for quota enforcement with unallocated reservations.
Date Tue, 12 Dec 2017 02:07:39 GMT

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



Thanks for testing this! I left comments in the first test that also apply to the second test,
so be sure to update both tests :)


src/tests/hierarchical_allocator_tests.cpp
Lines 1274-1280 (patched)
<https://reviews.apache.org/r/64493/#comment272039>

    Just curious, is it possible to template it by the `Resource::ReservationInfo::Type` enum?
    
    That might read a little more easily in the test output and code.
    
    In terms of naming, I think we tend to avoid naming these `SomeTestWithParam` (I only
see the one named like this in this file). Naming this way means that if we add another parameterized
test, we then need to add what it is parameterized by in the name:
    
    ```
    // Before:
    HierarchicalAllocatorTestWithParam ... bool (for quota)
    
    HierarchicalAllocatorTestWithQuotaParam ... bool (for quota)
    HierarchicalAllocatorTestWithReservationParam ... bool or Resource::ReservationInfo::Type
(for reservation)
    ```
    
    Then I think it can get confusing because one's test interpretation of a "quota parameter"
might be different than another's and you may then need to split the names further to distinguish?
    
    Maybe we could just name this fixture `HierarchicalAllocatorTestLimitWithReservations`
and have the following?
    
    ```
    TEST_P(HierarchicalAllocatorLimitWithReservationsTest, Unallocated)
    TEST_P(HierarchicalAllocatorTestWithReservationParam, Allocated)
    ```



src/tests/hierarchical_allocator_tests.cpp
Lines 1312 (patched)
<https://reviews.apache.org/r/64493/#comment272041>

    What is "ops"? If it's the principal I think the test would be more readable with s/"ops"/"principal"/



src/tests/hierarchical_allocator_tests.cpp
Lines 1314-1319 (patched)
<https://reviews.apache.org/r/64493/#comment272043>

    Is it possible just to inline this?
    
    ```
        AWAIT_READY(allocator->updateAvailable(agent1.id(), {RESERVE(reserved)}));
    ```



src/tests/hierarchical_allocator_tests.cpp
Lines 1323 (patched)
<https://reviews.apache.org/r/64493/#comment272044>

    Can you stringify QUOTA_ROLE instead of burning it in here?
    
    Alternatively, I think you can pass `Resources` in?
    
    ```
    Resources resources = Resources::parse("cpus:1;mem:1024).get();
    
    resources.push_reservation(a static reservation)
    
    agent1 = createSlaveInfo(resources);
    ```
    
    Hm.. this makes me wonder why we couldn't just do the following with the parameter?
    
    ```
    Resource::ReservationInfo reservation;
    reservation.set_type(GetParam());
    reservation.set_role(QUOTA_ROLE);
    
    Resources resources = Resources::parse("cpus:1;mem:1024").get();
    resources = resources.pushReservation(reservation);
    
    agent1 = createSlaveInfo(resources);
    
    ...
    ```
    
    No need for an if condition?



src/tests/hierarchical_allocator_tests.cpp
Lines 1339-1340 (patched)
<https://reviews.apache.org/r/64493/#comment272045>

    Can you set quota first? Right now there's a race in this test between the framework being
allocated resources vs the quota getting set.



src/tests/hierarchical_allocator_tests.cpp
Lines 1382-1386 (patched)
<https://reviews.apache.org/r/64493/#comment272046>

    Shouldn't this description be nearly identical to the one above except for unallocated
being allocated? It was a little surprising that this is phrased differently



src/tests/hierarchical_allocator_tests.cpp
Lines 1471-1473 (patched)
<https://reviews.apache.org/r/64493/#comment272047>

    Hm.. what is this testing? I was expecting to see that you couldn't go over your limit
when you have [un]allocated resesrvations. This test being the allocated case, the above test
being the unallocate case?


- Benjamin Mahler


On Dec. 11, 2017, 4:32 a.m., Meng Zhu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64493/
> -----------------------------------------------------------
> 
> (Updated Dec. 11, 2017, 4:32 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added tests for quota enforcement with unallocated reservations.
> 
> 
> Diffs
> -----
> 
>   src/tests/hierarchical_allocator_tests.cpp 862f4683da04d37d9fe9f471d6ec9cd7751f39ec

> 
> 
> Diff: https://reviews.apache.org/r/64493/diff/1/
> 
> 
> Testing
> -------
> 
> maek check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>


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