This is an automatically generated e-mail. To reply, visit:

How about a more succinct summary: "Traversed all roles for quota allocation"?

Changes touching allocator are vulnerable to races, especially in tests. Please 
extend the testing (and mention this in the "Testing Done" section) with 
something like like `GTEST_FILTER="*HierarchicalAllocatorTest*" 
./bin/mesos-tests.sh --gtest_shuffle --gtest-repeat`.

I think we can simplify the second test. Your intention is to test the 
scenario, where one role with quota and lower share is saturated while the 
other is not, and check whether the other will get offers, correct? (Btw, 
scenario description is a great comment for a test!). If so, I'd suggest to 
start with two agents, say "1resource", "2resources" and quotas "1resource", 
"4resources". This guarantees that the first role is 1)satisfied and 2)has a 
lower share. Then you add a third agent with, say, "2resources" and check that 
the second role gets resources offered. Do you think this is cleaner and more 

src/master/allocator/mesos/hierarchical.cpp (lines 1142 - 1144)

    ... to do any allocations for this role.

src/master/allocator/mesos/hierarchical.cpp (line 1156)

    s/allocations,/allocations for this role,

src/tests/hierarchical_allocator_tests.cpp (lines 1842 - 1844)

    How about this:
    This test checks that if one role with quota has no frameworks in it, other 
roles with quota are still offered resources.

src/tests/hierarchical_allocator_tests.cpp (line 1845)

    Do you think this one is better?

src/tests/hierarchical_allocator_tests.cpp (line 1849)

    It's minor, but I think we tend to put numerals at the back of the string, 
something like "quota-role-1".

src/tests/hierarchical_allocator_tests.cpp (line 1856)

    Any reason why you don't use the defacto-standard "cpus:2;mem:1024;disk:0"? 
I think some people may wonder why this test differs from the other.
    Same for the next test.

src/tests/hierarchical_allocator_tests.cpp (line 1860)

    How about: "Set quota for both roles".

src/tests/hierarchical_allocator_tests.cpp (lines 1862 - 1863)

    blank line, please.

src/tests/hierarchical_allocator_tests.cpp (line 1866)

    s/`framework`/a framework (no need for backticks)
    s/QUOTA_ROLE2/`QUOTA_ROLE2` (mind backticks).
    Same for the next test.

src/tests/hierarchical_allocator_tests.cpp (line 1871)

    How about "Due to the coarse-grained nature of the allocations, `framework` 
will get all `agent`'s resources."

src/tests/hierarchical_allocator_tests.cpp (lines 1879 - 1880)

    How about "This test checks that if there are multiple roles with quota all 
of them get enough offers given there are enough resources."

src/tests/hierarchical_allocator_tests.cpp (lines 1892 - 1893)

    Same question as above.

src/tests/hierarchical_allocator_tests.cpp (line 1894)

    Let's add a comment here about what is the initial state. For example, take 
a look at `DRFWithQuota` or `QuotaAgainstStarvation` tests.

src/tests/hierarchical_allocator_tests.cpp (line 1895)

    We should definitely drag reader's attention to the fact that quota is 
pretty different. How about: "Quota for `QUOTA_ROLE1` is 10 times smaller than 
for `QUOTA_ROLE2`."

src/tests/hierarchical_allocator_tests.cpp (lines 1897 - 1898)

    Blank line, please.

src/tests/hierarchical_allocator_tests.cpp (lines 1925 - 1937)

    Let's prepend this section with a comment. What is your intention here?

src/tests/hierarchical_allocator_tests.cpp (lines 1939 - 1940)

    If you want to wait for `addSlave()` to complete, please move it before 
recover section. Also, if you do not expect any allocations for addSalve, 
please explain why.

src/tests/hierarchical_allocator_tests.cpp (line 1941)

    Let's describe the cluster state here. Example from one of the existing 
      // Total cluster resources (1 agent): cpus=1, mem=512.
      // QUOTA_ROLE share = 0.25 (cpus=0.25, mem=128) [quota: cpus=0.25, 
      //   framework1 share = 1
      // NO_QUOTA_ROLE share = 0.75 (cpus=0.75, mem=384)
      //   framework2 share = 0

src/tests/hierarchical_allocator_tests.cpp (lines 1957 - 1958)

    We need explain why.

- Alexander Rukletsov

On Jan. 19, 2016, 10 a.m., Guangya Liu wrote:
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41769/
> -----------------------------------------------------------
> (Updated Jan. 19, 2016, 10 a.m.)
> Review request for mesos, Alexander Rukletsov, Joris Van Remoortere, Klaus 
> Ma, and Neil Conway.
> Bugs: MESOS-4411
>     https://issues.apache.org/jira/browse/MESOS-4411
> Repository: mesos
> Description
> -------
> This patch include two parts:
> 1) If there are some `non-active roles` in front of active roles after 
> `quotaRoleSorter`, when the allocator encounter a `non-active role`, the 
> allocator should not `break` but `continue` to allocate Quota for other 
> active roles to make sure other roles can get its quotaed resources.
> 2) If some role's quota reach its guaranteed value, the allocator should 
> handle another role but not break. Take the following case: role1 has quota 5 
> and got 5, role2 has quota 100 and got 50, the role1 will be put in front of 
> role2 by the `quotaRoleSorter`, if allocator `break` when found role1 is 
> satisfied, then role2 will never get its quotaed resources.
> Diffs
> -----
>   src/master/allocator/mesos/hierarchical.cpp 
> 72e69a0f42dd724713f2a7a75f1b92ef16eb5569 
>   src/tests/hierarchical_allocator_tests.cpp 
> 9362dd306497ba01e0f387c3862456cdcac6f863 
> Diff: https://reviews.apache.org/r/41769/diff/
> Testing
> -------
> make
> make check
> GLOG_v=2  ./bin/mesos-tests.sh  --gtest_filter="HierarchicalAllocatorTest.*" 
> --verbose --gtest_repeat=100 --gtest_shuffle
> Thanks,
> Guangya Liu

Reply via email to