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