> 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 > >
