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




src/tests/hierarchical_allocator_tests.cpp (line 2307)
<https://reviews.apache.org/r/41672/#comment181212>

    Let's abolish slavery in this test : ). Here and below in comments and 
variable namings.



src/tests/hierarchical_allocator_tests.cpp (lines 2320 - 2324)
<https://reviews.apache.org/r/41672/#comment181213>

    This is not related to "register agents with the same resources" section, 
right? Let's move it up and write why this is necessary.



src/tests/hierarchical_allocator_tests.cpp (line 2325)
<https://reviews.apache.org/r/41672/#comment181223>

    Why post-increment? Is it consistent to the codebase?



src/tests/hierarchical_allocator_tests.cpp (line 2329)
<https://reviews.apache.org/r/41672/#comment181215>

    Why not merging this loop with the previous one?



src/tests/hierarchical_allocator_tests.cpp (line 2335)
<https://reviews.apache.org/r/41672/#comment181214>

    `{}` will do the job.



src/tests/hierarchical_allocator_tests.cpp (line 2337)
<https://reviews.apache.org/r/41672/#comment181217>

    Here is a good place to insert a comment describing the cluster state. Look 
for an example in other tests.



src/tests/hierarchical_allocator_tests.cpp (line 2338)
<https://reviews.apache.org/r/41672/#comment181219>

    Backticks instead of single quotes, please! Here and everywhere.



src/tests/hierarchical_allocator_tests.cpp (line 2342)
<https://reviews.apache.org/r/41672/#comment181220>

    `{}` will do the job. Here and below.



src/tests/hierarchical_allocator_tests.cpp (lines 2350 - 2354)
<https://reviews.apache.org/r/41672/#comment181221>

    How about separating this in two sections: check allocation state and 
recovering resources?
    
    Between these sections you can insert a comment describing the cluster 
state (I've mentioned that above already).



src/tests/hierarchical_allocator_tests.cpp (lines 2371 - 2397)
<https://reviews.apache.org/r/41672/#comment181225>

    You can wrap this block in curly braces and avoid adding numerals to 
variable names. Same for the block below.



src/tests/hierarchical_allocator_tests.cpp (line 2373)
<https://reviews.apache.org/r/41672/#comment181229>

    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.



src/tests/hierarchical_allocator_tests.cpp (line 2374)
<https://reviews.apache.org/r/41672/#comment181224>

    Let's pull advancing clock above so that it's not lost between lines.



src/tests/hierarchical_allocator_tests.cpp (lines 2402 - 2403)
<https://reviews.apache.org/r/41672/#comment181230>

    If you use `set` as proposed above, you can check  set against set here.



src/tests/hierarchical_allocator_tests.cpp (lines 2413 - 2414)
<https://reviews.apache.org/r/41672/#comment181226>

    You can avoid this by wrapping this section in curly braces.



src/tests/hierarchical_allocator_tests.cpp (lines 2423 - 2433)
<https://reviews.apache.org/r/41672/#comment181231>

    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.


- Alexander Rukletsov


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