mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Meng Zhu <m...@mesosphere.io>
Subject Re: Review Request 64493: Added tests for quota enforcement with unallocated reservations.
Date Tue, 12 Dec 2017 21:45:44 GMT


> On Dec. 11, 2017, 6:07 p.m., Benjamin Mahler wrote:
> > src/tests/hierarchical_allocator_tests.cpp
> > Lines 1274-1280 (patched)
> > <https://reviews.apache.org/r/64493/diff/1/?file=1912389#file1912389line1274>
> >
> >     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)
> >     ```

Good suggestion. Done.


> On Dec. 11, 2017, 6:07 p.m., Benjamin Mahler wrote:
> > src/tests/hierarchical_allocator_tests.cpp
> > Lines 1314-1319 (patched)
> > <https://reviews.apache.org/r/64493/diff/1/?file=1912389#file1912389line1314>
> >
> >     Is it possible just to inline this?
> >     
> >     ```
> >         AWAIT_READY(allocator->updateAvailable(agent1.id(), {RESERVE(reserved)}));
> >     ```

Done.


> On Dec. 11, 2017, 6:07 p.m., Benjamin Mahler wrote:
> > src/tests/hierarchical_allocator_tests.cpp
> > Lines 1323 (patched)
> > <https://reviews.apache.org/r/64493/diff/1/?file=1912389#file1912389line1323>
> >
> >     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?

Great suggestion! Done, much concise.


> On Dec. 11, 2017, 6:07 p.m., Benjamin Mahler wrote:
> > src/tests/hierarchical_allocator_tests.cpp
> > Lines 1471-1473 (patched)
> > <https://reviews.apache.org/r/64493/diff/1/?file=1912389#file1912389line1471>
> >
> >     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?

The first test checks you couldn't go over your limit when you have unallocated reservation.
The second test checks that you can properly allocated up to your limit when you have partial
reservation.
The case of all allocated reservation should be no different from the case where there is
no reservations in the sense that they are just allocated resources. There are already test
for that.

The second test is needed to check against the tricky case of double counting of allocated-reservations.
(Hence the comment above the second test).


- Meng


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


On Dec. 12, 2017, 1:45 p.m., Meng Zhu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64493/
> -----------------------------------------------------------
> 
> (Updated Dec. 12, 2017, 1:45 p.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/2/
> 
> 
> Testing
> -------
> 
> maek check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>


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