mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Neil Conway" <neil.con...@gmail.com>
Subject Re: Review Request 41672: Test case(s) for weights + allocation behavior
Date Tue, 29 Dec 2015 18:10:45 GMT

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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message