----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43824/#review121642 -----------------------------------------------------------
Thanks for the follow-up! Let's move this test before all becnhmark tests from the `HierarchicalAllocator_BENCHMARK_Test` fixture. src/tests/hierarchical_allocator_tests.cpp (lines 2607 - 2609) <https://reviews.apache.org/r/43824/#comment183393> I'm an ESL, but having both "per weight" and "by weight" sounds a bit strange. Maybe Adam can help wiht finding the right preposition. src/tests/hierarchical_allocator_tests.cpp (lines 2612 - 2614) <https://reviews.apache.org/r/43824/#comment183401> We try to avoid naming the same entity differently, please use "batch allocation" instead of "periodic": ``` // Pausing the clock is not necessary, but ensures that the test // doesn't rely on the batch allocation in the allocator, which // would slow down the test. ``` src/tests/hierarchical_allocator_tests.cpp (lines 2631 - 2636) <https://reviews.apache.org/r/43824/#comment183394> Does it fit one line? I know we are a bit inconsistent about it, but let's prefer oneliners where it doesn't impact the readability. src/tests/hierarchical_allocator_tests.cpp (lines 2647 - 2649) <https://reviews.apache.org/r/43824/#comment183396> How about // Framework2 registers with 'role2' which also uses the default weight. It // will not get any offers because all resources are offered to `framework1`. src/tests/hierarchical_allocator_tests.cpp (lines 2653 - 2656) <https://reviews.apache.org/r/43824/#comment183398> Technically, allocation may not happen before `AWAIT_READY(allocation);` which calls `Clock::settle()`. Let's move this comment *after* the next code block. It is a minor change, but I think such details are utterly important for new people joining the project and reading the code. src/tests/hierarchical_allocator_tests.cpp (line 2682) <https://reviews.apache.org/r/43824/#comment183399> s/background allocation cycle/batch allocation src/tests/hierarchical_allocator_tests.cpp (line 2683) <https://reviews.apache.org/r/43824/#comment183402> This doesn't guarantee the allocation happened. Even though the clock will be implicitly settled below, let's explicitly add `Clock::settle()` here so that the next comment is 100% correct. src/tests/hierarchical_allocator_tests.cpp (lines 2724 - 2725) <https://reviews.apache.org/r/43824/#comment183404> "Their" is vague. I think you can safely kill the clause and leave just the first part of the sentence: "Update the weight of `framework2`'s role to 2.0" (mind backticks!) src/tests/hierarchical_allocator_tests.cpp (lines 2726 - 2728) <https://reviews.apache.org/r/43824/#comment183408> I think these lines are good candidate to go under the `{}` together with the checks. This way you can avoid numeral suffixes and have a 1:1 relation between `{}`-blocks and test cases. Does it make sense? src/tests/hierarchical_allocator_tests.cpp (line 2731) <https://reviews.apache.org/r/43824/#comment183406> Yes, but again we need to `Clock::settle()` for clarity : ) src/tests/hierarchical_allocator_tests.cpp (line 2738) <https://reviews.apache.org/r/43824/#comment183409> s/test to// src/tests/hierarchical_allocator_tests.cpp (lines 2749 - 2760) <https://reviews.apache.org/r/43824/#comment183415> Let me clarify what I meant in r/41672/. I would like to avoid the `if-else` dispatch based on the framework id. It becomes even worse below, where we have three frameworks. Here is what I suggest: ``` { hashmap<FrameworkID, Allocation> currentAllocations; Resources totalAllocatedResources; for (unsigned i = 0; i < 2; i++) { Future<Allocation> allocation = allocations.get(); AWAIT_READY(allocation); totalAllocatedResources += Resources::sum(allocation.get().resources); currentAllocations[allocation.get().frameworkId] = allocation.get(); // Recover the allocated resources so they can be offered again next time. foreachpair (const SlaveID& slaveId, const Resources& resources, allocation.get().resources) { allocator->recoverResources( allocation.get().frameworkId, slaveId, resources, None()); } } // ASSERT_EQ(2u, currentAllocations[framework1.id()].resources.size()); EXPECT_EQ(Resources::parse(DOUBLE_RESOURCES).get(), Resources::sum(currentAllocations[framework1.id()].resources)); // ASSERT_EQ(4u, currentAllocations[framework2.id()].resources.size()); EXPECT_EQ(Resources::parse(FOURFOLD_RESOURCES).get(), Resources::sum(currentAllocations[framework2.id()].resources)); // Check to ensure that these two allocations sum to the total resources, // this check can ensure there are only two allocations in this case. EXPECT_EQ(Resources::parse(TOTAL_RESOURCES).get(), totalAllocatedResources); EXPECT_EQ(hashset<FrameworkID>(expectedFrameworkIds), currentAllocations.keys()); } ``` Note I don't explicitly use `expectedFrameworkIds`. src/tests/hierarchical_allocator_tests.cpp (lines 2772 - 2773) <https://reviews.apache.org/r/43824/#comment183410> Blank line src/tests/hierarchical_allocator_tests.cpp (lines 2780 - 2782) <https://reviews.apache.org/r/43824/#comment183656> See my comment above about how to avoid numeral suffixes. src/tests/hierarchical_allocator_tests.cpp (line 2792) <https://reviews.apache.org/r/43824/#comment183658> But `settle()` is necessary : ) src/tests/hierarchical_allocator_tests.cpp (line 2839) <https://reviews.apache.org/r/43824/#comment183391> Why do we need to `resume` here? - Alexander Rukletsov On March 1, 2016, 6:42 a.m., Yongqiao Wang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43824/ > ----------------------------------------------------------- > > (Updated March 1, 2016, 6:42 a.m.) > > > Review request for mesos, Adam B and Alexander Rukletsov. > > > Bugs: MESOS-4200 > https://issues.apache.org/jira/browse/MESOS-4200 > > > Repository: mesos > > > Description > ------- > > Addressed comments of 41672. > > > Diffs > ----- > > src/tests/hierarchical_allocator_tests.cpp > 5f771f02db9bd098f3cd36730cd84bf2f5e87a33 > > Diff: https://reviews.apache.org/r/43824/diff/ > > > Testing > ------- > > make && make check successfully. > > > Thanks, > > Yongqiao Wang > >
