You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Klaus Ma <kl...@gmail.com> on 2016/01/19 09:50:08 UTC
Re: Review Request 42289: Quota doesn't allocate resources on slave
joining.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42289/
-----------------------------------------------------------
(Updated Jan. 19, 2016, 4:50 p.m.)
Review request for mesos, Alexander Rukletsov, Ben Mahler, Joris Van Remoortere, and Neil Conway.
Changes
-------
Update summary
Summary (updated)
-----------------
Quota doesn't allocate resources on slave joining.
Bugs: MESOS-4102
https://issues.apache.org/jira/browse/MESOS-4102
Repository: mesos
Description
-------
Calcuated 'remainingClusterResources' by all activated slaves.
Diffs
-----
src/master/allocator/mesos/hierarchical.cpp 48acde69b1a2f305b568a7e322a58708063dd30a
Diff: https://reviews.apache.org/r/42289/diff/
Testing
-------
make
make check
Thanks,
Klaus Ma
Re: Review Request 42289: Calcuated 'remainingClusterResources' by all
activated slaves.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42289/#review115264
-----------------------------------------------------------
Ship it!
Ship It!
- Alexander Rukletsov
On Jan. 19, 2016, 2:25 p.m., Klaus Ma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42289/
> -----------------------------------------------------------
>
> (Updated Jan. 19, 2016, 2:25 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Ben Mahler, Joris Van Remoortere, and Neil Conway.
>
>
> Bugs: MESOS-4102
> https://issues.apache.org/jira/browse/MESOS-4102
>
>
> Repository: mesos
>
>
> Description
> -------
>
> __Phenomenon__:
> Quota doesn't allocate resources on slave joining.
>
> __Root Cause__:
> Event-triggered allocations do not include all available agents. If we
> calculate remaining resources in the cluster using the partial view,
> we may overlook already laid away resources for quota'ed roles and lay
> away more. Hence we may unnecessarily deprive non-quota'ed frameworks
> of resources.
>
> Refer to AlexR's comments for more detail:
> https://issues.apache.org/jira/browse/MESOS-4102?focusedCommentId=15048495&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15048495
>
> __Solution/Fix__:
> Calcuated 'remainingClusterResources' by all activated slaves.
>
>
> Diffs
> -----
>
> src/master/allocator/mesos/hierarchical.cpp 48acde69b1a2f305b568a7e322a58708063dd30a
> src/tests/hierarchical_allocator_tests.cpp 9362dd306497ba01e0f387c3862456cdcac6f863
>
> Diff: https://reviews.apache.org/r/42289/diff/
>
>
> Testing
> -------
>
> make
> make check
> Mac & Ubuntu: ./bin/mesos-tests.sh --gtest_filter="HierarchicalAllocatorTest.*" --verbose --gtest_repeat=100 --gtest_shuffle
>
>
> Thanks,
>
> Klaus Ma
>
>
Re: Review Request 42289: Calcuated 'remainingClusterResources' by all
activated slaves.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On Jan. 19, 2016, 2:38 p.m., Joerg Schad wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 1229
> > <https://reviews.apache.org/r/42289/diff/4/?file=1201517#file1201517line1229>
> >
> > s/those visible in the current / those agents provided via `slaveIDs` to the `allocate` call.
Addressed in https://reviews.apache.org/r/42510/
> On Jan. 19, 2016, 2:38 p.m., Joerg Schad wrote:
> > src/tests/hierarchical_allocator_tests.cpp, line 1798
> > <https://reviews.apache.org/r/42289/diff/4/?file=1201518#file1201518line1798>
> >
> > Can we maybe --either here or below (e.g. l 1823)-- add some description that we are also testing the behavior on addSlave()?
Addressed in https://reviews.apache.org/r/42510/
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42289/#review115156
-----------------------------------------------------------
On Jan. 19, 2016, 2:25 p.m., Klaus Ma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42289/
> -----------------------------------------------------------
>
> (Updated Jan. 19, 2016, 2:25 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Ben Mahler, Joris Van Remoortere, and Neil Conway.
>
>
> Bugs: MESOS-4102
> https://issues.apache.org/jira/browse/MESOS-4102
>
>
> Repository: mesos
>
>
> Description
> -------
>
> __Phenomenon__:
> Quota doesn't allocate resources on slave joining.
>
> __Root Cause__:
> Event-triggered allocations do not include all available agents. If we
> calculate remaining resources in the cluster using the partial view,
> we may overlook already laid away resources for quota'ed roles and lay
> away more. Hence we may unnecessarily deprive non-quota'ed frameworks
> of resources.
>
> Refer to AlexR's comments for more detail:
> https://issues.apache.org/jira/browse/MESOS-4102?focusedCommentId=15048495&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15048495
>
> __Solution/Fix__:
> Calcuated 'remainingClusterResources' by all activated slaves.
>
>
> Diffs
> -----
>
> src/master/allocator/mesos/hierarchical.cpp 48acde69b1a2f305b568a7e322a58708063dd30a
> src/tests/hierarchical_allocator_tests.cpp 9362dd306497ba01e0f387c3862456cdcac6f863
>
> Diff: https://reviews.apache.org/r/42289/diff/
>
>
> Testing
> -------
>
> make
> make check
> Mac & Ubuntu: ./bin/mesos-tests.sh --gtest_filter="HierarchicalAllocatorTest.*" --verbose --gtest_repeat=100 --gtest_shuffle
>
>
> Thanks,
>
> Klaus Ma
>
>
Re: Review Request 42289: Calcuated 'remainingClusterResources' by all
activated slaves.
Posted by Joerg Schad <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42289/#review115156
-----------------------------------------------------------
src/master/allocator/mesos/hierarchical.cpp (line 1229)
<https://reviews.apache.org/r/42289/#comment176049>
s/those visible in the current / those agents provided via `slaveIDs` to the `allocate` call.
src/tests/hierarchical_allocator_tests.cpp (line 1798)
<https://reviews.apache.org/r/42289/#comment176046>
Can we maybe --either here or below (e.g. l 1823)-- add some description that we are also testing the behavior on addSlave()?
- Joerg Schad
On Jan. 19, 2016, 2:25 p.m., Klaus Ma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42289/
> -----------------------------------------------------------
>
> (Updated Jan. 19, 2016, 2:25 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Ben Mahler, Joris Van Remoortere, and Neil Conway.
>
>
> Bugs: MESOS-4102
> https://issues.apache.org/jira/browse/MESOS-4102
>
>
> Repository: mesos
>
>
> Description
> -------
>
> __Phenomenon__:
> Quota doesn't allocate resources on slave joining.
>
> __Root Cause__:
> Event-triggered allocations do not include all available agents. If we
> calculate remaining resources in the cluster using the partial view,
> we may overlook already laid away resources for quota'ed roles and lay
> away more. Hence we may unnecessarily deprive non-quota'ed frameworks
> of resources.
>
> Refer to AlexR's comments for more detail:
> https://issues.apache.org/jira/browse/MESOS-4102?focusedCommentId=15048495&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15048495
>
> __Solution/Fix__:
> Calcuated 'remainingClusterResources' by all activated slaves.
>
>
> Diffs
> -----
>
> src/master/allocator/mesos/hierarchical.cpp 48acde69b1a2f305b568a7e322a58708063dd30a
> src/tests/hierarchical_allocator_tests.cpp 9362dd306497ba01e0f387c3862456cdcac6f863
>
> Diff: https://reviews.apache.org/r/42289/diff/
>
>
> Testing
> -------
>
> make
> make check
> Mac & Ubuntu: ./bin/mesos-tests.sh --gtest_filter="HierarchicalAllocatorTest.*" --verbose --gtest_repeat=100 --gtest_shuffle
>
>
> Thanks,
>
> Klaus Ma
>
>
Re: Review Request 42289: Calcuated 'remainingClusterResources' by all
activated slaves.
Posted by Joris Van Remoortere <jo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42289/#review115270
-----------------------------------------------------------
Ship it!
Ship It!
- Joris Van Remoortere
On Jan. 19, 2016, 2:25 p.m., Klaus Ma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42289/
> -----------------------------------------------------------
>
> (Updated Jan. 19, 2016, 2:25 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Ben Mahler, Joris Van Remoortere, and Neil Conway.
>
>
> Bugs: MESOS-4102
> https://issues.apache.org/jira/browse/MESOS-4102
>
>
> Repository: mesos
>
>
> Description
> -------
>
> __Phenomenon__:
> Quota doesn't allocate resources on slave joining.
>
> __Root Cause__:
> Event-triggered allocations do not include all available agents. If we
> calculate remaining resources in the cluster using the partial view,
> we may overlook already laid away resources for quota'ed roles and lay
> away more. Hence we may unnecessarily deprive non-quota'ed frameworks
> of resources.
>
> Refer to AlexR's comments for more detail:
> https://issues.apache.org/jira/browse/MESOS-4102?focusedCommentId=15048495&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15048495
>
> __Solution/Fix__:
> Calcuated 'remainingClusterResources' by all activated slaves.
>
>
> Diffs
> -----
>
> src/master/allocator/mesos/hierarchical.cpp 48acde69b1a2f305b568a7e322a58708063dd30a
> src/tests/hierarchical_allocator_tests.cpp 9362dd306497ba01e0f387c3862456cdcac6f863
>
> Diff: https://reviews.apache.org/r/42289/diff/
>
>
> Testing
> -------
>
> make
> make check
> Mac & Ubuntu: ./bin/mesos-tests.sh --gtest_filter="HierarchicalAllocatorTest.*" --verbose --gtest_repeat=100 --gtest_shuffle
>
>
> Thanks,
>
> Klaus Ma
>
>
Re: Review Request 42289: Calcuated 'remainingClusterResources' by all
activated slaves.
Posted by Klaus Ma <kl...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42289/
-----------------------------------------------------------
(Updated Jan. 19, 2016, 10:25 p.m.)
Review request for mesos, Alexander Rukletsov, Ben Mahler, Joris Van Remoortere, and Neil Conway.
Changes
-------
Address comments.
Bugs: MESOS-4102
https://issues.apache.org/jira/browse/MESOS-4102
Repository: mesos
Description
-------
__Phenomenon__:
Quota doesn't allocate resources on slave joining.
__Root Cause__:
Event-triggered allocations do not include all available agents. If we
calculate remaining resources in the cluster using the partial view,
we may overlook already laid away resources for quota'ed roles and lay
away more. Hence we may unnecessarily deprive non-quota'ed frameworks
of resources.
Refer to AlexR's comments for more detail:
https://issues.apache.org/jira/browse/MESOS-4102?focusedCommentId=15048495&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15048495
__Solution/Fix__:
Calcuated 'remainingClusterResources' by all activated slaves.
Diffs (updated)
-----
src/master/allocator/mesos/hierarchical.cpp 48acde69b1a2f305b568a7e322a58708063dd30a
src/tests/hierarchical_allocator_tests.cpp 9362dd306497ba01e0f387c3862456cdcac6f863
Diff: https://reviews.apache.org/r/42289/diff/
Testing (updated)
-------
make
make check
Mac & Ubuntu: ./bin/mesos-tests.sh --gtest_filter="HierarchicalAllocatorTest.*" --verbose --gtest_repeat=100 --gtest_shuffle
Thanks,
Klaus Ma
Re: Review Request 42289: Calcuated 'remainingClusterResources' by all
activated slaves.
Posted by Klaus Ma <kl...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42289/
-----------------------------------------------------------
(Updated Jan. 19, 2016, 8:03 p.m.)
Review request for mesos, Alexander Rukletsov, Ben Mahler, Joris Van Remoortere, and Neil Conway.
Changes
-------
Address comments
Bugs: MESOS-4102
https://issues.apache.org/jira/browse/MESOS-4102
Repository: mesos
Description
-------
__Phenomenon__:
Quota doesn't allocate resources on slave joining.
__Root Cause__:
Event-triggered allocations do not include all available agents. If we
calculate remaining resources in the cluster using the partial view,
we may overlook already laid away resources for quota'ed roles and lay
away more. Hence we may unnecessarily deprive non-quota'ed frameworks
of resources.
Refer to AlexR's comments for more detail:
https://issues.apache.org/jira/browse/MESOS-4102?focusedCommentId=15048495&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15048495
__Solution/Fix__:
Calcuated 'remainingClusterResources' by all activated slaves.
Diffs (updated)
-----
src/master/allocator/mesos/hierarchical.cpp 48acde69b1a2f305b568a7e322a58708063dd30a
src/tests/hierarchical_allocator_tests.cpp 9362dd306497ba01e0f387c3862456cdcac6f863
Diff: https://reviews.apache.org/r/42289/diff/
Testing
-------
make
make check
./bin/mesos-tests.sh --gtest_filter="HierarchicalAllocatorTest.*" --verbose --gtest_repeat=100 --gtest_shuffle
Thanks,
Klaus Ma
Re: Review Request 42289: Calcuated 'remainingClusterResources' by all
activated slaves.
Posted by Klaus Ma <kl...@gmail.com>.
> On Jan. 19, 2016, 7:53 p.m., Guangya Liu wrote:
> > src/tests/hierarchical_allocator_tests.cpp, lines 1817-1829
> > <https://reviews.apache.org/r/42289/diff/3/?file=1201483#file1201483line1817>
> >
> > What about make the test cases cover both `addSlave()` before and after `createFrameworkInfo`?
The case after `createFrameworkInfo` is enough to cover both cases.
- Klaus
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42289/#review115143
-----------------------------------------------------------
On Jan. 19, 2016, 8:03 p.m., Klaus Ma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42289/
> -----------------------------------------------------------
>
> (Updated Jan. 19, 2016, 8:03 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Ben Mahler, Joris Van Remoortere, and Neil Conway.
>
>
> Bugs: MESOS-4102
> https://issues.apache.org/jira/browse/MESOS-4102
>
>
> Repository: mesos
>
>
> Description
> -------
>
> __Phenomenon__:
> Quota doesn't allocate resources on slave joining.
>
> __Root Cause__:
> Event-triggered allocations do not include all available agents. If we
> calculate remaining resources in the cluster using the partial view,
> we may overlook already laid away resources for quota'ed roles and lay
> away more. Hence we may unnecessarily deprive non-quota'ed frameworks
> of resources.
>
> Refer to AlexR's comments for more detail:
> https://issues.apache.org/jira/browse/MESOS-4102?focusedCommentId=15048495&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15048495
>
> __Solution/Fix__:
> Calcuated 'remainingClusterResources' by all activated slaves.
>
>
> Diffs
> -----
>
> src/master/allocator/mesos/hierarchical.cpp 48acde69b1a2f305b568a7e322a58708063dd30a
> src/tests/hierarchical_allocator_tests.cpp 9362dd306497ba01e0f387c3862456cdcac6f863
>
> Diff: https://reviews.apache.org/r/42289/diff/
>
>
> Testing
> -------
>
> make
> make check
> ./bin/mesos-tests.sh --gtest_filter="HierarchicalAllocatorTest.*" --verbose --gtest_repeat=100 --gtest_shuffle
>
>
> Thanks,
>
> Klaus Ma
>
>
Re: Review Request 42289: Calcuated 'remainingClusterResources' by all
activated slaves.
Posted by Guangya Liu <gy...@gmail.com>.
> On 一月 19, 2016, 11:53 a.m., Guangya Liu wrote:
> > src/tests/hierarchical_allocator_tests.cpp, lines 1817-1829
> > <https://reviews.apache.org/r/42289/diff/3/?file=1201483#file1201483line1817>
> >
> > What about make the test cases cover both `addSlave()` before and after `createFrameworkInfo`?
>
> Klaus Ma wrote:
> The case after `createFrameworkInfo` is enough to cover both cases.
Because this test is testing the cases that the resources added after create framework can be also allocated to framework, I think that it is better to keep `addSlave` cases `before` and `after` create framework to make sure that the resources can be allocated to framework for both conditions.
- Guangya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42289/#review115143
-----------------------------------------------------------
On 一月 19, 2016, 2:25 p.m., Klaus Ma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42289/
> -----------------------------------------------------------
>
> (Updated 一月 19, 2016, 2:25 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Ben Mahler, Joris Van Remoortere, and Neil Conway.
>
>
> Bugs: MESOS-4102
> https://issues.apache.org/jira/browse/MESOS-4102
>
>
> Repository: mesos
>
>
> Description
> -------
>
> __Phenomenon__:
> Quota doesn't allocate resources on slave joining.
>
> __Root Cause__:
> Event-triggered allocations do not include all available agents. If we
> calculate remaining resources in the cluster using the partial view,
> we may overlook already laid away resources for quota'ed roles and lay
> away more. Hence we may unnecessarily deprive non-quota'ed frameworks
> of resources.
>
> Refer to AlexR's comments for more detail:
> https://issues.apache.org/jira/browse/MESOS-4102?focusedCommentId=15048495&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15048495
>
> __Solution/Fix__:
> Calcuated 'remainingClusterResources' by all activated slaves.
>
>
> Diffs
> -----
>
> src/master/allocator/mesos/hierarchical.cpp 48acde69b1a2f305b568a7e322a58708063dd30a
> src/tests/hierarchical_allocator_tests.cpp 9362dd306497ba01e0f387c3862456cdcac6f863
>
> Diff: https://reviews.apache.org/r/42289/diff/
>
>
> Testing
> -------
>
> make
> make check
> Mac & Ubuntu: ./bin/mesos-tests.sh --gtest_filter="HierarchicalAllocatorTest.*" --verbose --gtest_repeat=100 --gtest_shuffle
>
>
> Thanks,
>
> Klaus Ma
>
>
Re: Review Request 42289: Calcuated 'remainingClusterResources' by all
activated slaves.
Posted by Guangya Liu <gy...@gmail.com>.
> On 一月 19, 2016, 11:53 a.m., Guangya Liu wrote:
> > src/tests/hierarchical_allocator_tests.cpp, lines 1817-1829
> > <https://reviews.apache.org/r/42289/diff/3/?file=1201483#file1201483line1817>
> >
> > What about make the test cases cover both `addSlave()` before and after `createFrameworkInfo`?
>
> Klaus Ma wrote:
> The case after `createFrameworkInfo` is enough to cover both cases.
>
> Guangya Liu wrote:
> Because this test is testing the cases that the resources added after create framework can be also allocated to framework, I think that it is better to keep `addSlave` cases `before` and `after` create framework to make sure that the resources can be allocated to framework for both conditions.
>
> Alexander Rukletsov wrote:
> If we add an agent *before* adding any frameworks, we may lay away resources, but no allocations will be made. Currently we can't really look into the allocator and check whether the resources have been actually laid away. However, once we have at least one framework, allocations can be made. Hence I would agree with Klaus that one test case is enough, but we want to make sure people understand that the order is important and that we actually test two things with one test.
>
> I'll think about how we can improve comments around the test.
Do you mean `addSlave` and `createFramework` order? Does the order still important after this patch?
- Guangya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42289/#review115143
-----------------------------------------------------------
On 一月 19, 2016, 2:25 p.m., Klaus Ma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42289/
> -----------------------------------------------------------
>
> (Updated 一月 19, 2016, 2:25 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Ben Mahler, Joris Van Remoortere, and Neil Conway.
>
>
> Bugs: MESOS-4102
> https://issues.apache.org/jira/browse/MESOS-4102
>
>
> Repository: mesos
>
>
> Description
> -------
>
> __Phenomenon__:
> Quota doesn't allocate resources on slave joining.
>
> __Root Cause__:
> Event-triggered allocations do not include all available agents. If we
> calculate remaining resources in the cluster using the partial view,
> we may overlook already laid away resources for quota'ed roles and lay
> away more. Hence we may unnecessarily deprive non-quota'ed frameworks
> of resources.
>
> Refer to AlexR's comments for more detail:
> https://issues.apache.org/jira/browse/MESOS-4102?focusedCommentId=15048495&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15048495
>
> __Solution/Fix__:
> Calcuated 'remainingClusterResources' by all activated slaves.
>
>
> Diffs
> -----
>
> src/master/allocator/mesos/hierarchical.cpp 48acde69b1a2f305b568a7e322a58708063dd30a
> src/tests/hierarchical_allocator_tests.cpp 9362dd306497ba01e0f387c3862456cdcac6f863
>
> Diff: https://reviews.apache.org/r/42289/diff/
>
>
> Testing
> -------
>
> make
> make check
> Mac & Ubuntu: ./bin/mesos-tests.sh --gtest_filter="HierarchicalAllocatorTest.*" --verbose --gtest_repeat=100 --gtest_shuffle
>
>
> Thanks,
>
> Klaus Ma
>
>
Re: Review Request 42289: Calcuated 'remainingClusterResources' by all
activated slaves.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On Jan. 19, 2016, 11:53 a.m., Guangya Liu wrote:
> > src/tests/hierarchical_allocator_tests.cpp, lines 1817-1829
> > <https://reviews.apache.org/r/42289/diff/3/?file=1201483#file1201483line1817>
> >
> > What about make the test cases cover both `addSlave()` before and after `createFrameworkInfo`?
>
> Klaus Ma wrote:
> The case after `createFrameworkInfo` is enough to cover both cases.
>
> Guangya Liu wrote:
> Because this test is testing the cases that the resources added after create framework can be also allocated to framework, I think that it is better to keep `addSlave` cases `before` and `after` create framework to make sure that the resources can be allocated to framework for both conditions.
>
> Alexander Rukletsov wrote:
> If we add an agent *before* adding any frameworks, we may lay away resources, but no allocations will be made. Currently we can't really look into the allocator and check whether the resources have been actually laid away. However, once we have at least one framework, allocations can be made. Hence I would agree with Klaus that one test case is enough, but we want to make sure people understand that the order is important and that we actually test two things with one test.
>
> I'll think about how we can improve comments around the test.
>
> Guangya Liu wrote:
> Do you mean `addSlave` and `createFramework` order? Does the order still important after this patch?
It is important to prove the patch fixes the issue : ).
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42289/#review115143
-----------------------------------------------------------
On Jan. 19, 2016, 2:25 p.m., Klaus Ma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42289/
> -----------------------------------------------------------
>
> (Updated Jan. 19, 2016, 2:25 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Ben Mahler, Joris Van Remoortere, and Neil Conway.
>
>
> Bugs: MESOS-4102
> https://issues.apache.org/jira/browse/MESOS-4102
>
>
> Repository: mesos
>
>
> Description
> -------
>
> __Phenomenon__:
> Quota doesn't allocate resources on slave joining.
>
> __Root Cause__:
> Event-triggered allocations do not include all available agents. If we
> calculate remaining resources in the cluster using the partial view,
> we may overlook already laid away resources for quota'ed roles and lay
> away more. Hence we may unnecessarily deprive non-quota'ed frameworks
> of resources.
>
> Refer to AlexR's comments for more detail:
> https://issues.apache.org/jira/browse/MESOS-4102?focusedCommentId=15048495&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15048495
>
> __Solution/Fix__:
> Calcuated 'remainingClusterResources' by all activated slaves.
>
>
> Diffs
> -----
>
> src/master/allocator/mesos/hierarchical.cpp 48acde69b1a2f305b568a7e322a58708063dd30a
> src/tests/hierarchical_allocator_tests.cpp 9362dd306497ba01e0f387c3862456cdcac6f863
>
> Diff: https://reviews.apache.org/r/42289/diff/
>
>
> Testing
> -------
>
> make
> make check
> Mac & Ubuntu: ./bin/mesos-tests.sh --gtest_filter="HierarchicalAllocatorTest.*" --verbose --gtest_repeat=100 --gtest_shuffle
>
>
> Thanks,
>
> Klaus Ma
>
>
Re: Review Request 42289: Calcuated 'remainingClusterResources' by all
activated slaves.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On Jan. 19, 2016, 11:53 a.m., Guangya Liu wrote:
> > src/tests/hierarchical_allocator_tests.cpp, lines 1817-1829
> > <https://reviews.apache.org/r/42289/diff/3/?file=1201483#file1201483line1817>
> >
> > What about make the test cases cover both `addSlave()` before and after `createFrameworkInfo`?
>
> Klaus Ma wrote:
> The case after `createFrameworkInfo` is enough to cover both cases.
>
> Guangya Liu wrote:
> Because this test is testing the cases that the resources added after create framework can be also allocated to framework, I think that it is better to keep `addSlave` cases `before` and `after` create framework to make sure that the resources can be allocated to framework for both conditions.
If we add an agent *before* adding any frameworks, we may lay away resources, but no allocations will be made. Currently we can't really look into the allocator and check whether the resources have been actually laid away. However, once we have at least one framework, allocations can be made. Hence I would agree with Klaus that one test case is enough, but we want to make sure people understand that the order is important and that we actually test two things with one test.
I'll think about how we can improve comments around the test.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42289/#review115143
-----------------------------------------------------------
On Jan. 19, 2016, 2:25 p.m., Klaus Ma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42289/
> -----------------------------------------------------------
>
> (Updated Jan. 19, 2016, 2:25 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Ben Mahler, Joris Van Remoortere, and Neil Conway.
>
>
> Bugs: MESOS-4102
> https://issues.apache.org/jira/browse/MESOS-4102
>
>
> Repository: mesos
>
>
> Description
> -------
>
> __Phenomenon__:
> Quota doesn't allocate resources on slave joining.
>
> __Root Cause__:
> Event-triggered allocations do not include all available agents. If we
> calculate remaining resources in the cluster using the partial view,
> we may overlook already laid away resources for quota'ed roles and lay
> away more. Hence we may unnecessarily deprive non-quota'ed frameworks
> of resources.
>
> Refer to AlexR's comments for more detail:
> https://issues.apache.org/jira/browse/MESOS-4102?focusedCommentId=15048495&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15048495
>
> __Solution/Fix__:
> Calcuated 'remainingClusterResources' by all activated slaves.
>
>
> Diffs
> -----
>
> src/master/allocator/mesos/hierarchical.cpp 48acde69b1a2f305b568a7e322a58708063dd30a
> src/tests/hierarchical_allocator_tests.cpp 9362dd306497ba01e0f387c3862456cdcac6f863
>
> Diff: https://reviews.apache.org/r/42289/diff/
>
>
> Testing
> -------
>
> make
> make check
> Mac & Ubuntu: ./bin/mesos-tests.sh --gtest_filter="HierarchicalAllocatorTest.*" --verbose --gtest_repeat=100 --gtest_shuffle
>
>
> Thanks,
>
> Klaus Ma
>
>
Re: Review Request 42289: Calcuated 'remainingClusterResources' by all
activated slaves.
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42289/#review115143
-----------------------------------------------------------
src/tests/hierarchical_allocator_tests.cpp (lines 1812 - 1824)
<https://reviews.apache.org/r/42289/#comment176030>
What about make the test cases cover both `addSlave()` before and after `createFrameworkInfo`?
src/tests/hierarchical_allocator_tests.cpp (line 1818)
<https://reviews.apache.org/r/42289/#comment176029>
4 spaces
- Guangya Liu
On 一月 19, 2016, 11:10 a.m., Klaus Ma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42289/
> -----------------------------------------------------------
>
> (Updated 一月 19, 2016, 11:10 a.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Ben Mahler, Joris Van Remoortere, and Neil Conway.
>
>
> Bugs: MESOS-4102
> https://issues.apache.org/jira/browse/MESOS-4102
>
>
> Repository: mesos
>
>
> Description
> -------
>
> __Phenomenon__:
> Quota doesn't allocate resources on slave joining.
>
> __Root Cause__:
> Event-triggered allocations do not include all available agents. If we
> calculate remaining resources in the cluster using the partial view,
> we may overlook already laid away resources for quota'ed roles and lay
> away more. Hence we may unnecessarily deprive non-quota'ed frameworks
> of resources.
>
> Refer to AlexR's comments for more detail:
> https://issues.apache.org/jira/browse/MESOS-4102?focusedCommentId=15048495&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15048495
>
> __Solution/Fix__:
> Calcuated 'remainingClusterResources' by all activated slaves.
>
>
> Diffs
> -----
>
> src/master/allocator/mesos/hierarchical.cpp 48acde69b1a2f305b568a7e322a58708063dd30a
> src/tests/hierarchical_allocator_tests.cpp 9362dd306497ba01e0f387c3862456cdcac6f863
>
> Diff: https://reviews.apache.org/r/42289/diff/
>
>
> Testing
> -------
>
> make
> make check
> ./bin/mesos-tests.sh --gtest_filter="HierarchicalAllocatorTest.*" --verbose --gtest_repeat=100 --gtest_shuffle
>
>
> Thanks,
>
> Klaus Ma
>
>
Re: Review Request 42289: Calcuated 'remainingClusterResources' by all
activated slaves.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42289/#review115142
-----------------------------------------------------------
It would be great if you add a comment describing the cluster state, have a look at other tests for an example!
src/master/allocator/mesos/hierarchical.cpp (line 1230)
<https://reviews.apache.org/r/42289/#comment176026>
Backtick allocate() please.
src/tests/hierarchical_allocator_tests.cpp (lines 1823 - 1824)
<https://reviews.apache.org/r/42289/#comment176028>
Let's add a `NOTE:` here that each `addSlave()` triggers an event-based allocation.
src/tests/hierarchical_allocator_tests.cpp (lines 1826 - 1828)
<https://reviews.apache.org/r/42289/#comment176027>
Any reason why you change the original comment?
- Alexander Rukletsov
On Jan. 19, 2016, 11:10 a.m., Klaus Ma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42289/
> -----------------------------------------------------------
>
> (Updated Jan. 19, 2016, 11:10 a.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Ben Mahler, Joris Van Remoortere, and Neil Conway.
>
>
> Bugs: MESOS-4102
> https://issues.apache.org/jira/browse/MESOS-4102
>
>
> Repository: mesos
>
>
> Description
> -------
>
> __Phenomenon__:
> Quota doesn't allocate resources on slave joining.
>
> __Root Cause__:
> Event-triggered allocations do not include all available agents. If we
> calculate remaining resources in the cluster using the partial view,
> we may overlook already laid away resources for quota'ed roles and lay
> away more. Hence we may unnecessarily deprive non-quota'ed frameworks
> of resources.
>
> Refer to AlexR's comments for more detail:
> https://issues.apache.org/jira/browse/MESOS-4102?focusedCommentId=15048495&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15048495
>
> __Solution/Fix__:
> Calcuated 'remainingClusterResources' by all activated slaves.
>
>
> Diffs
> -----
>
> src/master/allocator/mesos/hierarchical.cpp 48acde69b1a2f305b568a7e322a58708063dd30a
> src/tests/hierarchical_allocator_tests.cpp 9362dd306497ba01e0f387c3862456cdcac6f863
>
> Diff: https://reviews.apache.org/r/42289/diff/
>
>
> Testing
> -------
>
> make
> make check
> ./bin/mesos-tests.sh --gtest_filter="HierarchicalAllocatorTest.*" --verbose --gtest_repeat=100 --gtest_shuffle
>
>
> Thanks,
>
> Klaus Ma
>
>
Re: Review Request 42289: Calcuated 'remainingClusterResources' by all
activated slaves.
Posted by Klaus Ma <kl...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42289/
-----------------------------------------------------------
(Updated Jan. 19, 2016, 7:10 p.m.)
Review request for mesos, Alexander Rukletsov, Ben Mahler, Joris Van Remoortere, and Neil Conway.
Changes
-------
Repeated test done for allocator.
Bugs: MESOS-4102
https://issues.apache.org/jira/browse/MESOS-4102
Repository: mesos
Description
-------
__Phenomenon__:
Quota doesn't allocate resources on slave joining.
__Root Cause__:
Event-triggered allocations do not include all available agents. If we
calculate remaining resources in the cluster using the partial view,
we may overlook already laid away resources for quota'ed roles and lay
away more. Hence we may unnecessarily deprive non-quota'ed frameworks
of resources.
Refer to AlexR's comments for more detail:
https://issues.apache.org/jira/browse/MESOS-4102?focusedCommentId=15048495&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15048495
__Solution/Fix__:
Calcuated 'remainingClusterResources' by all activated slaves.
Diffs
-----
src/master/allocator/mesos/hierarchical.cpp 48acde69b1a2f305b568a7e322a58708063dd30a
src/tests/hierarchical_allocator_tests.cpp 9362dd306497ba01e0f387c3862456cdcac6f863
Diff: https://reviews.apache.org/r/42289/diff/
Testing (updated)
-------
make
make check
./bin/mesos-tests.sh --gtest_filter="HierarchicalAllocatorTest.*" --verbose --gtest_repeat=100 --gtest_shuffle
Thanks,
Klaus Ma
Re: Review Request 42289: Calcuated 'remainingClusterResources' by all
activated slaves.
Posted by Klaus Ma <kl...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42289/
-----------------------------------------------------------
(Updated Jan. 19, 2016, 6:55 p.m.)
Review request for mesos, Alexander Rukletsov, Ben Mahler, Joris Van Remoortere, and Neil Conway.
Changes
-------
Address comments.
Summary (updated)
-----------------
Calcuated 'remainingClusterResources' by all activated slaves.
Bugs: MESOS-4102
https://issues.apache.org/jira/browse/MESOS-4102
Repository: mesos
Description (updated)
-------
__Phenomenon__:
Quota doesn't allocate resources on slave joining.
__Root Cause__:
Event-triggered allocations do not include all available agents. If we
calculate remaining resources in the cluster using the partial view,
we may overlook already laid away resources for quota'ed roles and lay
away more. Hence we may unnecessarily deprive non-quota'ed frameworks
of resources.
Refer to AlexR's comments for more detail:
https://issues.apache.org/jira/browse/MESOS-4102?focusedCommentId=15048495&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15048495
__Solution/Fix__:
Calcuated 'remainingClusterResources' by all activated slaves.
Diffs (updated)
-----
src/master/allocator/mesos/hierarchical.cpp 48acde69b1a2f305b568a7e322a58708063dd30a
src/tests/hierarchical_allocator_tests.cpp 9362dd306497ba01e0f387c3862456cdcac6f863
Diff: https://reviews.apache.org/r/42289/diff/
Testing
-------
make
make check
Thanks,
Klaus Ma
Re: Review Request 42289: Quota doesn't allocate resources on slave
joining.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On Jan. 19, 2016, 9:23 a.m., Alexander Rukletsov wrote:
> > Let's tweak some wording and testing and we are good to go!
> >
> > I liked the initial summary more. IMO a patch should describe the solution, and not the problem. It's quite opposite for JIRA tickets, hence I'm convinced patches and tickets should not share the same title. How about "Calcuated 'remainingClusterResources' using all available agents."
> >
> > I also think it won't hurt to extend the description and explain why we need this change. How about
> > "Event-triggered allocations do not include all available agents. If we
> > calculate remaining resources in the cluster using the partial view,
> > we may overlook already laid away resources for quota'ed roles and lay
> > away more. Hence we may unnecessarily deprive non-quota'ed frameworks
> > of resources."
> >
> > Changes touching allocator are vulnerable to races, especially in tests. Please extend the testing (and mention this in the "Testing Done" section) with goodies like `GTEST_FILTER="*HierarchicalAllocatorTest*" ./bin/mesos-tests.sh --gtest_shuffle --gtest-repeat`.
> >
> > Let's add a test for this change. Ideally the test should fail without the change and pass it with the change. I think Neil has already provided the test in the ticket, could you please include it?
Maybe even better, you can modify existing `QuotaAbsentFramework` test.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42289/#review115132
-----------------------------------------------------------
On Jan. 19, 2016, 8:50 a.m., Klaus Ma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42289/
> -----------------------------------------------------------
>
> (Updated Jan. 19, 2016, 8:50 a.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Ben Mahler, Joris Van Remoortere, and Neil Conway.
>
>
> Bugs: MESOS-4102
> https://issues.apache.org/jira/browse/MESOS-4102
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Calcuated 'remainingClusterResources' by all activated slaves.
>
>
> Diffs
> -----
>
> src/master/allocator/mesos/hierarchical.cpp 48acde69b1a2f305b568a7e322a58708063dd30a
>
> Diff: https://reviews.apache.org/r/42289/diff/
>
>
> Testing
> -------
>
> make
> make check
>
>
> Thanks,
>
> Klaus Ma
>
>
Re: Review Request 42289: Calcuated 'remainingClusterResources' by all
activated slaves.
Posted by Klaus Ma <kl...@gmail.com>.
> On Jan. 19, 2016, 5:23 p.m., Alexander Rukletsov wrote:
> > Let's tweak some wording and testing and we are good to go!
> >
> > I liked the initial summary more. IMO a patch should describe the solution, and not the problem. It's quite opposite for JIRA tickets, hence I'm convinced patches and tickets should not share the same title. How about "Calcuated 'remainingClusterResources' using all available agents."
> >
> > I also think it won't hurt to extend the description and explain why we need this change. How about
> > "Event-triggered allocations do not include all available agents. If we
> > calculate remaining resources in the cluster using the partial view,
> > we may overlook already laid away resources for quota'ed roles and lay
> > away more. Hence we may unnecessarily deprive non-quota'ed frameworks
> > of resources."
> >
> > Changes touching allocator are vulnerable to races, especially in tests. Please extend the testing (and mention this in the "Testing Done" section) with goodies like `GTEST_FILTER="*HierarchicalAllocatorTest*" ./bin/mesos-tests.sh --gtest_shuffle --gtest-repeat`.
> >
> > Let's add a test for this change. Ideally the test should fail without the change and pass it with the change. I think Neil has already provided the test in the ticket, could you please include it?
>
> Alexander Rukletsov wrote:
> Maybe even better, you can modify existing `QuotaAbsentFramework` test.
Yes, that's what I'm thinking :). Current patch passed Neil's test in Mac OS, I'll re-check it in Ubuntu for `./bin/mesos-tests.sh`; it seems perf did not support no-Linux system.
- Klaus
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42289/#review115132
-----------------------------------------------------------
On Jan. 19, 2016, 6:55 p.m., Klaus Ma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42289/
> -----------------------------------------------------------
>
> (Updated Jan. 19, 2016, 6:55 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Ben Mahler, Joris Van Remoortere, and Neil Conway.
>
>
> Bugs: MESOS-4102
> https://issues.apache.org/jira/browse/MESOS-4102
>
>
> Repository: mesos
>
>
> Description
> -------
>
> __Phenomenon__:
> Quota doesn't allocate resources on slave joining.
>
> __Root Cause__:
> Event-triggered allocations do not include all available agents. If we
> calculate remaining resources in the cluster using the partial view,
> we may overlook already laid away resources for quota'ed roles and lay
> away more. Hence we may unnecessarily deprive non-quota'ed frameworks
> of resources.
>
> Refer to AlexR's comments for more detail:
> https://issues.apache.org/jira/browse/MESOS-4102?focusedCommentId=15048495&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15048495
>
> __Solution/Fix__:
> Calcuated 'remainingClusterResources' by all activated slaves.
>
>
> Diffs
> -----
>
> src/master/allocator/mesos/hierarchical.cpp 48acde69b1a2f305b568a7e322a58708063dd30a
> src/tests/hierarchical_allocator_tests.cpp 9362dd306497ba01e0f387c3862456cdcac6f863
>
> Diff: https://reviews.apache.org/r/42289/diff/
>
>
> Testing
> -------
>
> make
> make check
>
>
> Thanks,
>
> Klaus Ma
>
>
Re: Review Request 42289: Quota doesn't allocate resources on slave
joining.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42289/#review115132
-----------------------------------------------------------
Let's tweak some wording and testing and we are good to go!
I liked the initial summary more. IMO a patch should describe the solution, and not the problem. It's quite opposite for JIRA tickets, hence I'm convinced patches and tickets should not share the same title. How about "Calcuated 'remainingClusterResources' using all available agents."
I also think it won't hurt to extend the description and explain why we need this change. How about
"Event-triggered allocations do not include all available agents. If we
calculate remaining resources in the cluster using the partial view,
we may overlook already laid away resources for quota'ed roles and lay
away more. Hence we may unnecessarily deprive non-quota'ed frameworks
of resources."
Changes touching allocator are vulnerable to races, especially in tests. Please extend the testing (and mention this in the "Testing Done" section) with goodies like `GTEST_FILTER="*HierarchicalAllocatorTest*" ./bin/mesos-tests.sh --gtest_shuffle --gtest-repeat`.
Let's add a test for this change. Ideally the test should fail without the change and pass it with the change. I think Neil has already provided the test in the ticket, could you please include it?
src/master/allocator/mesos/hierarchical.cpp (lines 1227 - 1228)
<https://reviews.apache.org/r/42289/#comment175992>
Let's add a note here that we iterate over all slaves and why. How about
// NOTE: We use all active agents and not just those visible in the current `allocate()` call so that frameworks in roles without quota are not unnecessarily deprived of resources.
- Alexander Rukletsov
On Jan. 19, 2016, 8:50 a.m., Klaus Ma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42289/
> -----------------------------------------------------------
>
> (Updated Jan. 19, 2016, 8:50 a.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Ben Mahler, Joris Van Remoortere, and Neil Conway.
>
>
> Bugs: MESOS-4102
> https://issues.apache.org/jira/browse/MESOS-4102
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Calcuated 'remainingClusterResources' by all activated slaves.
>
>
> Diffs
> -----
>
> src/master/allocator/mesos/hierarchical.cpp 48acde69b1a2f305b568a7e322a58708063dd30a
>
> Diff: https://reviews.apache.org/r/42289/diff/
>
>
> Testing
> -------
>
> make
> make check
>
>
> Thanks,
>
> Klaus Ma
>
>