> On Feb. 19, 2016, 9:31 a.m., Alexander Rukletsov wrote:
> >
> 
> Adam B wrote:
>     Yongqiao, since Alex didn't get his review in before I committed the 
> patch, could you create a new patch addressing his feedback and link to it 
> from the comments here? Thanks.

Thanks Adam and Alex, I have posted patch https://reviews.apache.org/r/43824/ 
to address the comments as below.


> On Feb. 19, 2016, 9:31 a.m., Alexander Rukletsov wrote:
> > src/tests/hierarchical_allocator_tests.cpp, line 2338
> > <https://reviews.apache.org/r/41672/diff/12/?file=1203046#file1203046line2338>
> >
> >     Backticks instead of single quotes, please! Here and everywhere.

I find single quote?apostrophe and backticks are all used in our comments, You 
can check this file and others like `master_quota_tests.cpp` 
`master_maintenance_tests.cpp`, etc. Cloud you tell me what is the rule for 
using them in comments, I will follow up later. Thanks.


> On Feb. 19, 2016, 9:31 a.m., Alexander Rukletsov wrote:
> > src/tests/hierarchical_allocator_tests.cpp, lines 2423-2433
> > <https://reviews.apache.org/r/41672/diff/12/?file=1203046#file1203046line2423>
> >
> >     Will it be cleaner to put resources from the allocation into a hashmap 
> > by framework id? I think this way you can even get rid of the loop.
> >     
> >     Same suggestions for the section below.

Personallly, the current implementation is cleaner, if we using a loop for 
this, we also need to check the framework Id in the loop due to there 
allocation size and resources are different.


> On Feb. 19, 2016, 9:31 a.m., Alexander Rukletsov wrote:
> > src/tests/hierarchical_allocator_tests.cpp, line 2373
> > <https://reviews.apache.org/r/41672/diff/12/?file=1203046#file1203046line2373>
> >
> >     You can put framework id's into a `std::set` instead. Given there are 
> > two allocations, there is a guarantee, that there will be no attempts to 
> > put the same framework into the set twice.

In the current implementation, we only need to check the allocation size for 
each framework, it does not care the size of whole contianer (currentt is 
hashmap), so I think use hashmap can meet the current requirement.


> On Feb. 19, 2016, 9:31 a.m., Alexander Rukletsov wrote:
> > src/tests/hierarchical_allocator_tests.cpp, lines 2402-2403
> > <https://reviews.apache.org/r/41672/diff/12/?file=1203046#file1203046line2402>
> >
> >     If you use `set` as proposed above, you can check  set against set here.

Sorry to do not understand your suggestion here, current using hashmap and use 
frameworkid as it's key, here to check value for allocation number for each 
framework.


- Yongqiao


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41672/#review119839
-----------------------------------------------------------


On Jan. 20, 2016, 8:36 a.m., Yongqiao Wang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41672/
> -----------------------------------------------------------
> 
> (Updated Jan. 20, 2016, 8:36 a.m.)
> 
> 
> Review request for mesos, Adam B, Neil Conway, and Qian Zhang.
> 
> 
> Bugs: MESOS-4200
>     https://issues.apache.org/jira/browse/MESOS-4200
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Test case(s) for weights + allocation behaviour.
> 
> 
> Diffs
> -----
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 953712149bd951789beb29c72779c4ac65aa48dc 
> 
> Diff: https://reviews.apache.org/r/41672/diff/
> 
> 
> Testing
> -------
> 
> Make check done:
> $ ./src/mesos-tests --gtest_filter=HierarchicalAllocatorTest.UpdateWeight
> Source directory: /Users/yqwyq/Desktop/mesos
> Build directory: /Users/yqwyq/Desktop/mesos/build
> [==========] Running 1 test from 1 test case.
> [----------] Global test environment set-up.
> [----------] 1 test from HierarchicalAllocatorTest
> [ RUN      ] HierarchicalAllocatorTest.UpdateWeight
> [       OK ] HierarchicalAllocatorTest.UpdateWeight (87 ms)
> [----------] 1 test from HierarchicalAllocatorTest (87 ms total)
> 
> [----------] Global test environment tear-down
> [==========] 1 test from 1 test case ran. (176 ms total)
> [  PASSED  ] 1 test.
> 
> 
> Thanks,
> 
> Yongqiao Wang
> 
>

Reply via email to