----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71639/#review218460 -----------------------------------------------------------
Fix it, then Ship it! src/tests/hierarchical_allocator_tests.cpp Lines 8384-8393 (patched) <https://reviews.apache.org/r/71639/#comment306191> Not sure that this is a right place for this comment, especially the part describing what each reservation contains. After all, things stated in this comment are not a responsibility of the `ResourceParam` struct. Rather, this is how `UpdateAllocation` behcnmark uses these parameters (which is described in the comments below). src/tests/hierarchical_allocator_tests.cpp Lines 8408 (patched) <https://reviews.apache.org/r/71639/#comment306189> Shouldn't that be "exactly", not "more than"? Also, "unique"/"distinct"/"random" would probably be a better word than "various" (IMO, "various" does not indicate that all the individual `Resource`s have different metadata to make them non-addable). src/tests/hierarchical_allocator_tests.cpp Lines 8425-8427 (patched) <https://reviews.apache.org/r/71639/#comment306190> I would suggest slightly rewording this: "Each set of reservations contains some cpu, memory, disk and port (number of ranges is controlled by `portRangeCount`) with a random label, and is allocated to a framework." src/tests/hierarchical_allocator_tests.cpp Lines 8429 (patched) <https://reviews.apache.org/r/71639/#comment306188> s/a "loop" framework/one of the frameworks/ ? src/tests/hierarchical_allocator_tests.cpp Lines 8510 (patched) <https://reviews.apache.org/r/71639/#comment306187> Outdated comment? (There is no dedicated loop role now.) src/tests/hierarchical_allocator_tests.cpp Lines 8569 (patched) <https://reviews.apache.org/r/71639/#comment306186> If we want to print agent resources size, shouldn't that be `param.reservationCount * param.roleCount * RESOURCE_NAMES.size()` or `agentResources.size()`? Alternatively, words `Agent resources size` can be replaced by more precise `Total number of reservations`. - Andrei Sekretenko On Oct. 29, 2019, 3:43 a.m., Meng Zhu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71639/ > ----------------------------------------------------------- > > (Updated Oct. 29, 2019, 3:43 a.m.) > > > Review request for mesos, Andrei Sekretenko and Benjamin Mahler. > > > Bugs: MESOS-10016 > https://issues.apache.org/jira/browse/MESOS-10016 > > > Repository: mesos > > > Description > ------- > > This benchmark evaluates the performance of > `allocator->UpdateAllocation()` where the agent has various > sizes of resource reservations. > > > Diffs > ----- > > src/tests/hierarchical_allocator_tests.cpp > 38fd19cee6409e4daa5bb0ab523e8e464cdcc9a5 > > > Diff: https://reviews.apache.org/r/71639/diff/3/ > > > Testing > ------- > > On the master branch optimized build: > > ``` > [ RUN ] > ResourceParam/HierarchicalAllocator__BENCHMARK_WithResourceParam.UpdateAllocation/0 > Agent resources size: 50 (50 roles, 1 reservations per role, 1 ranges per > port resource) > 20 RESERVE operations took 1.968569746secs, each takes 98.428487ms > 20 UNRESERVE operations took 1.978470742secs, each takes 98.923537ms > [ OK ] > ResourceParam/HierarchicalAllocator__BENCHMARK_WithResourceParam.UpdateAllocation/0 > (3996 ms) > [ RUN ] > ResourceParam/HierarchicalAllocator__BENCHMARK_WithResourceParam.UpdateAllocation/1 > Agent resources size: 100 (100 roles, 1 reservations per role, 1 ranges per > port resource) > 20 RESERVE operations took 10.094084426secs, each takes 504.704221ms > 20 UNRESERVE operations took 10.293614069secs, each takes 514.680703ms > [ OK ] > ResourceParam/HierarchicalAllocator__BENCHMARK_WithResourceParam.UpdateAllocation/1 > (20584 ms) > [ RUN ] > ResourceParam/HierarchicalAllocator__BENCHMARK_WithResourceParam.UpdateAllocation/2 > Agent resources size: 200 (200 roles, 1 reservations per role, 1 ranges per > port resource) > 20 RESERVE operations took 1.16018705933333mins, each takes 3.480561178secs > 20 UNRESERVE operations took 1.17209781845mins, each takes 3.516293455secs > [ OK ] > ResourceParam/HierarchicalAllocator__BENCHMARK_WithResourceParam.UpdateAllocation/2 > (141363 ms) > ``` > > > Thanks, > > Meng Zhu > >
