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

Reply via email to