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


Overall comments: I think we can make the test a bit more concise and focused 
on testing weight-related allocation behavior. Specifically:

1. Don't bother testing that when there is a single framework, it is allocated 
all the resources (other tests cover this already).
2. Test that if we have two frameworks in two roles with 3:1 weight ratio, 
resources are allocated accordingly. I believe we don't need to check three 
allocations in a loop.
3. Test what happens when we add a new role with a weight of 2.


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

    Whitespace.



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

    "Framework framework1" is unnecessary stuttering. Can we say "framework1" 
registers with "role1" instead?



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

    Whitespace.



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

    Typo.



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

    I think it would be clearer to move `counts` inside the outer loop body, 
and then removing the explicit `clear()` calls.



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

    What is the value of looping three times? i.e., do we have any reason to 
believe that looping additional times will catch more bugs? Conversely, why is 
looping three times enough? I would probably opt for removing the loop.



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

    "index" does not seem like the best variable name. How about "iteration", 
or "j"?



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

    Typo.



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

    We should assert that `allocation.get().frameworkId == framework2.id()`



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

    "reses" is not a word. How about "resources"?


commens

- Neil Conway


On Dec. 29, 2015, 7:21 a.m., Yongqiao Wang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41672/
> -----------------------------------------------------------
> 
> (Updated Dec. 29, 2015, 7:21 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 behavior
> 
> 
> Diffs
> -----
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> e044f832c2c16e53e663c6ced5452649bb0dcb59 
> 
> Diff: https://reviews.apache.org/r/41672/diff/
> 
> 
> Testing
> -------
> 
> Make check done:
> Yongs-MacBook-Pro:build yqwyq$ ./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 (257 ms)
> [----------] 1 test from HierarchicalAllocatorTest (257 ms total)
> 
> [----------] Global test environment tear-down
> [==========] 1 test from 1 test case ran. (340 ms total)
> [  PASSED  ] 1 test.
> 
> 
> Thanks,
> 
> Yongqiao Wang
> 
>

Reply via email to