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