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 70390: Added a test to ensure correct headroom accounting.
Date Fri, 05 Apr 2019 00:34:57 GMT


> On April 4, 2019, 3:02 p.m., Benjamin Mahler wrote:
> > Does this test fail on master prior to the fixes?

It does not pass with the current master.


> On April 4, 2019, 3:02 p.m., Benjamin Mahler wrote:
> > src/tests/hierarchical_allocator_tests.cpp
> > Lines 6176-6178 (patched)
> > <https://reviews.apache.org/r/70390/diff/1/?file=2137502#file2137502line6176>
> >
> >     just as an FYI, when I read this I was expecting it would test:
> >     
> >     "a" <-- has quota
> >     "a/b"
> >     
> >     (where it ensures that "a/b"'s allocation counts towards "a"'s quota)
> >     
> >     I guess we don't have a test for this, but it looks like a variant of 'QuotaWithNestedRoleReservation'
will accomplish it.
> >     
> >     Maybe call this one QuotaHeadroomWithNested...

Changed the name to `QuotaHeadroomWithNestedRoleAllocation`.


> On April 4, 2019, 3:02 p.m., Benjamin Mahler wrote:
> > src/tests/hierarchical_allocator_tests.cpp
> > Lines 6180-6193 (patched)
> > <https://reviews.apache.org/r/70390/diff/1/?file=2137502#file2137502line6180>
> >
> >     I found that while debugging some of these tests, the "imperative" style of
explaining them (e.g. do x then y then x) makes the tests a bit tougher to understand, and
that "declarative" explanations might be more readable? E.g.
> >     
> >     ```
> >     // Setup:
> >     //   agents: 2 * R
> >     //    roles: "a"   --> allocated R
> >     //           "a/b" --> allocated R
> >     //           "quota-role" --> guarantee R w/ no framework
> >     //
> >     // Test: Add 1 more agent with R.
> >     //       Ensure agent is held back for "quota-role".
> >     //       Add 1 more agent with R.
> >     //       Ensure only 1 of the two extra agents goes to "a" or "a/b"
> >     //         (since there is enough headroom for "quota-role")
> >     ```
> >     
> >     Here I can more easily see what setup state looks like and what is then tested
off of that setup.

Sounds good. Thanks!


> On April 4, 2019, 3:02 p.m., Benjamin Mahler wrote:
> > src/tests/hierarchical_allocator_tests.cpp
> > Lines 6199-6200 (patched)
> > <https://reviews.apache.org/r/70390/diff/1/?file=2137502#file2137502line6199>
> >
> >     I realize it's not just this test, but this brace syntax but it seems less readable
than:
> >     
> >     const string CHILD_ROLE = "a/b";
> >     
> >     Does that not compile?

Changed to `const string CHILD_ROLE = "a/b";`

I think, in general, the initialization list is preferred over the assignment when initializing
objects. But yeah, here the assignment seems more readable. Updated.


- Meng


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


On April 4, 2019, 1:31 p.m., Meng Zhu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70390/
> -----------------------------------------------------------
> 
> (Updated April 4, 2019, 1:31 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This test verifies that the headroom accounting is correct in the
> presence of subrole allocation. In particular, we need to ensure
> that a subrole's allocation is accounted only once (not multiple
> times in itself as well as all of its ancestors) when calculating
> available the quota headroom.
> 
> 
> Diffs
> -----
> 
>   src/tests/hierarchical_allocator_tests.cpp c7edee2f49bfac0796c9506265d3bb766416ee63

> 
> 
> Diff: https://reviews.apache.org/r/70390/diff/2/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>


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