> On March 3, 2016, 9:03 a.m., Alexander Rukletsov wrote: > > src/tests/hierarchical_allocator_tests.cpp, lines 2759-2770 > > <https://reviews.apache.org/r/43824/diff/2/?file=1275001#file1275001line2759> > > > > 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`.
A better name for `currentAllocations` is probably `frameworkAllocations`. We can also add extra checks, if you want: `ASSERT_TRUE(currentAllocations.contains(framework1.id()));` And there is a typo in the note above, it should read: "Note I don't explicitly use `frameworkIds` set." - Alexander ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43824/#review121642 ----------------------------------------------------------- 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 > >
