mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alexander Rukletsov" <ruklet...@gmail.com>
Subject Re: Review Request 41769: Made allocator traverse all roles for quota allocation.
Date Tue, 19 Jan 2016 11:11:31 GMT

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


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


src/master/allocator/mesos/hierarchical.cpp (lines 1142 - 1144)
<https://reviews.apache.org/r/41769/#comment175999>

    ... to do any allocations for this role.



src/master/allocator/mesos/hierarchical.cpp (line 1156)
<https://reviews.apache.org/r/41769/#comment176000>

    s/allocations,/allocations for this role,



src/tests/hierarchical_allocator_tests.cpp (lines 1842 - 1844)
<https://reviews.apache.org/r/41769/#comment175997>

    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)
<https://reviews.apache.org/r/41769/#comment175998>

    Do you think this one is better?
    s/MultiQuotaAbsentFramework/MultiQuotaAbsentFrameworks



src/tests/hierarchical_allocator_tests.cpp (line 1849)
<https://reviews.apache.org/r/41769/#comment176009>

    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)
<https://reviews.apache.org/r/41769/#comment176002>

    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)
<https://reviews.apache.org/r/41769/#comment176006>

    How about: "Set quota for both roles".



src/tests/hierarchical_allocator_tests.cpp (lines 1862 - 1863)
<https://reviews.apache.org/r/41769/#comment176004>

    blank line, please.



src/tests/hierarchical_allocator_tests.cpp (line 1866)
<https://reviews.apache.org/r/41769/#comment176005>

    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)
<https://reviews.apache.org/r/41769/#comment176007>

    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)
<https://reviews.apache.org/r/41769/#comment176008>

    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)
<https://reviews.apache.org/r/41769/#comment176010>

    Same question as above.



src/tests/hierarchical_allocator_tests.cpp (line 1894)
<https://reviews.apache.org/r/41769/#comment176019>

    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)
<https://reviews.apache.org/r/41769/#comment176017>

    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)
<https://reviews.apache.org/r/41769/#comment176011>

    Blank line, please.



src/tests/hierarchical_allocator_tests.cpp (lines 1925 - 1937)
<https://reviews.apache.org/r/41769/#comment176020>

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



src/tests/hierarchical_allocator_tests.cpp (lines 1939 - 1940)
<https://reviews.apache.org/r/41769/#comment176021>

    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)
<https://reviews.apache.org/r/41769/#comment176022>

    Let's describe the cluster state here. Example from one of the existing tests:
    ```
      // Total cluster resources (1 agent): cpus=1, mem=512.
      // QUOTA_ROLE share = 0.25 (cpus=0.25, mem=128) [quota: cpus=0.25, mem=128]
      //   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)
<https://reviews.apache.org/r/41769/#comment176023>

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


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