You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Benjamin Bannier <be...@mesosphere.io> on 2019/02/04 22:05:09 UTC
Review Request 69890: Added test for per-framework,
per-role minimal allocatable resources.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69890/
-----------------------------------------------------------
Review request for mesos, Benjamin Mahler and Meng Zhu.
Bugs: MESOS-9523
https://issues.apache.org/jira/browse/MESOS-9523
Repository: mesos
Description
-------
This patch adds tests for the behavior of per-framework, per-role
minimal allocatable resources. We validate the behavior of framework
minimal allocatable resources below the globally configured limits and
the behavior of empty filters or minimal resource quantities set up for
a framework.
Diffs
-----
src/tests/hierarchical_allocator_tests.cpp cc88afbad1b4e6bf707cb13b50c964aa01f9a3ee
Diff: https://reviews.apache.org/r/69890/diff/1/
Testing
-------
`make check`
Thanks,
Benjamin Bannier
Re: Review Request 69890: Added test for per-framework,
per-role minimal allocatable resources.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On Feb. 8, 2019, 9:47 p.m., Benjamin Mahler wrote:
> > src/tests/hierarchical_allocator_tests.cpp
> > Lines 2357-2374 (patched)
> > <https://reviews.apache.org/r/69890/diff/2/?file=2124579#file2124579line2357>
> >
> > Both "empty set" and "NullResourcesAllocatable" don't seem to accurately describe what's done?
> >
> > // Add a framework which specifies minimum allocatable resources
> > // with a single, empty resource.
> >
> > Single empty seems more appropriate? Come to think of it, should we validate that all entries are non-empty? There doesn't seem to be any point in allowing an empty entry, since any single empty entry renders it equivalent to the empty set case?
>
> Benjamin Bannier wrote:
> Fixed the test docstring.
>
> I think there is some value in allowing this going forward. An empty set of quantities maps onto an empty set of requirements while a set with a single, empty quantity maps onto a requirement for anything. WDYT?
>
> Dropping for now, please feel free to reopen.
>
> Benjamin Mahler wrote:
> Well, what's different about those the two requirements? They sound the same?
>
> Benjamin Bannier wrote:
> I updated the validation code in https://reviews.apache.org/r/69862/ to reject such empty quantity settings now, and removed the two test cases here.
Updated to only reject empty quantities itself, not empty min allocatable resource as well.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69890/#review212678
-----------------------------------------------------------
On Feb. 14, 2019, 9:48 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69890/
> -----------------------------------------------------------
>
> (Updated Feb. 14, 2019, 9:48 p.m.)
>
>
> Review request for mesos, Benjamin Mahler and Meng Zhu.
>
>
> Bugs: MESOS-9523
> https://issues.apache.org/jira/browse/MESOS-9523
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds tests for the behavior of per-framework, per-role
> minimal allocatable resources. We validate the behavior of framework
> minimal allocatable resources below the globally configured limits and
> the behavior of empty filters or minimal resource quantities set up for
> a framework.
>
>
> Diffs
> -----
>
> src/tests/hierarchical_allocator_tests.cpp 1366cc51332484d7ff3b5e5b7ba767efb97172c0
>
>
> Diff: https://reviews.apache.org/r/69890/diff/5/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 69890: Added test for per-framework,
per-role minimal allocatable resources.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On Feb. 8, 2019, 9:47 p.m., Benjamin Mahler wrote:
> > src/tests/hierarchical_allocator_tests.cpp
> > Lines 2357-2374 (patched)
> > <https://reviews.apache.org/r/69890/diff/2/?file=2124579#file2124579line2357>
> >
> > Both "empty set" and "NullResourcesAllocatable" don't seem to accurately describe what's done?
> >
> > // Add a framework which specifies minimum allocatable resources
> > // with a single, empty resource.
> >
> > Single empty seems more appropriate? Come to think of it, should we validate that all entries are non-empty? There doesn't seem to be any point in allowing an empty entry, since any single empty entry renders it equivalent to the empty set case?
Fixed the test docstring.
I think there is some value in allowing this going forward. An empty set of quantities maps onto an empty set of requirements while a set with a single, empty quantity maps onto a requirement for anything. WDYT?
Dropping for now, please feel free to reopen.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69890/#review212678
-----------------------------------------------------------
On Feb. 11, 2019, 4:46 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69890/
> -----------------------------------------------------------
>
> (Updated Feb. 11, 2019, 4:46 p.m.)
>
>
> Review request for mesos, Benjamin Mahler and Meng Zhu.
>
>
> Bugs: MESOS-9523
> https://issues.apache.org/jira/browse/MESOS-9523
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds tests for the behavior of per-framework, per-role
> minimal allocatable resources. We validate the behavior of framework
> minimal allocatable resources below the globally configured limits and
> the behavior of empty filters or minimal resource quantities set up for
> a framework.
>
>
> Diffs
> -----
>
> src/tests/hierarchical_allocator_tests.cpp cc88afbad1b4e6bf707cb13b50c964aa01f9a3ee
>
>
> Diff: https://reviews.apache.org/r/69890/diff/3/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 69890: Added test for per-framework,
per-role minimal allocatable resources.
Posted by Benjamin Mahler <bm...@apache.org>.
> On Feb. 8, 2019, 8:47 p.m., Benjamin Mahler wrote:
> > src/tests/hierarchical_allocator_tests.cpp
> > Lines 2357-2374 (patched)
> > <https://reviews.apache.org/r/69890/diff/2/?file=2124579#file2124579line2357>
> >
> > Both "empty set" and "NullResourcesAllocatable" don't seem to accurately describe what's done?
> >
> > // Add a framework which specifies minimum allocatable resources
> > // with a single, empty resource.
> >
> > Single empty seems more appropriate? Come to think of it, should we validate that all entries are non-empty? There doesn't seem to be any point in allowing an empty entry, since any single empty entry renders it equivalent to the empty set case?
>
> Benjamin Bannier wrote:
> Fixed the test docstring.
>
> I think there is some value in allowing this going forward. An empty set of quantities maps onto an empty set of requirements while a set with a single, empty quantity maps onto a requirement for anything. WDYT?
>
> Dropping for now, please feel free to reopen.
Well, what's different about those the two requirements? They sound the same?
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69890/#review212678
-----------------------------------------------------------
On Feb. 11, 2019, 3:46 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69890/
> -----------------------------------------------------------
>
> (Updated Feb. 11, 2019, 3:46 p.m.)
>
>
> Review request for mesos, Benjamin Mahler and Meng Zhu.
>
>
> Bugs: MESOS-9523
> https://issues.apache.org/jira/browse/MESOS-9523
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds tests for the behavior of per-framework, per-role
> minimal allocatable resources. We validate the behavior of framework
> minimal allocatable resources below the globally configured limits and
> the behavior of empty filters or minimal resource quantities set up for
> a framework.
>
>
> Diffs
> -----
>
> src/tests/hierarchical_allocator_tests.cpp cc88afbad1b4e6bf707cb13b50c964aa01f9a3ee
>
>
> Diff: https://reviews.apache.org/r/69890/diff/3/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 69890: Added test for per-framework,
per-role minimal allocatable resources.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On Feb. 8, 2019, 9:47 p.m., Benjamin Mahler wrote:
> > src/tests/hierarchical_allocator_tests.cpp
> > Lines 2357-2374 (patched)
> > <https://reviews.apache.org/r/69890/diff/2/?file=2124579#file2124579line2357>
> >
> > Both "empty set" and "NullResourcesAllocatable" don't seem to accurately describe what's done?
> >
> > // Add a framework which specifies minimum allocatable resources
> > // with a single, empty resource.
> >
> > Single empty seems more appropriate? Come to think of it, should we validate that all entries are non-empty? There doesn't seem to be any point in allowing an empty entry, since any single empty entry renders it equivalent to the empty set case?
>
> Benjamin Bannier wrote:
> Fixed the test docstring.
>
> I think there is some value in allowing this going forward. An empty set of quantities maps onto an empty set of requirements while a set with a single, empty quantity maps onto a requirement for anything. WDYT?
>
> Dropping for now, please feel free to reopen.
>
> Benjamin Mahler wrote:
> Well, what's different about those the two requirements? They sound the same?
I updated the validation code in https://reviews.apache.org/r/69862/ to reject such empty quantity settings now, and removed the two test cases here.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69890/#review212678
-----------------------------------------------------------
On Feb. 13, 2019, 11:31 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69890/
> -----------------------------------------------------------
>
> (Updated Feb. 13, 2019, 11:31 p.m.)
>
>
> Review request for mesos, Benjamin Mahler and Meng Zhu.
>
>
> Bugs: MESOS-9523
> https://issues.apache.org/jira/browse/MESOS-9523
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds tests for the behavior of per-framework, per-role
> minimal allocatable resources. We validate the behavior of framework
> minimal allocatable resources below the globally configured limits and
> the behavior of empty filters or minimal resource quantities set up for
> a framework.
>
>
> Diffs
> -----
>
> src/tests/hierarchical_allocator_tests.cpp 1366cc51332484d7ff3b5e5b7ba767efb97172c0
>
>
> Diff: https://reviews.apache.org/r/69890/diff/4/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 69890: Added test for per-framework,
per-role minimal allocatable resources.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69890/#review212678
-----------------------------------------------------------
Fix it, then Ship it!
Looks good, after leaving the comment below, I'm left wondering if there's any point in allowing an empty entry. Seems we may want to consider rejecting that during validation?
src/tests/hierarchical_allocator_tests.cpp
Lines 2357-2374 (patched)
<https://reviews.apache.org/r/69890/#comment298526>
Both "empty set" and "NullResourcesAllocatable" don't seem to accurately describe what's done?
// Add a framework which specifies minimum allocatable resources
// with a single, empty resource.
Single empty seems more appropriate? Come to think of it, should we validate that all entries are non-empty? There doesn't seem to be any point in allowing an empty entry, since any single empty entry renders it equivalent to the empty set case?
- Benjamin Mahler
On Feb. 8, 2019, 11:32 a.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69890/
> -----------------------------------------------------------
>
> (Updated Feb. 8, 2019, 11:32 a.m.)
>
>
> Review request for mesos, Benjamin Mahler and Meng Zhu.
>
>
> Bugs: MESOS-9523
> https://issues.apache.org/jira/browse/MESOS-9523
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds tests for the behavior of per-framework, per-role
> minimal allocatable resources. We validate the behavior of framework
> minimal allocatable resources below the globally configured limits and
> the behavior of empty filters or minimal resource quantities set up for
> a framework.
>
>
> Diffs
> -----
>
> src/tests/hierarchical_allocator_tests.cpp cc88afbad1b4e6bf707cb13b50c964aa01f9a3ee
>
>
> Diff: https://reviews.apache.org/r/69890/diff/2/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 69890: Added test for per-framework,
per-role minimal allocatable resources.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69890/#review212666
-----------------------------------------------------------
FAIL: Failed to apply the dependent review: 69821.
Failed command: `python.exe .\support\apply-reviews.py -n -r 69821`
All the build artifacts available at: http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2866/mesos-review-69890
Relevant logs:
- [apply-review-69821.log](http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2866/mesos-review-69890/logs/apply-review-69821.log):
```
error: patch failed: src/master/allocator/mesos/hierarchical.cpp:1832
error: src/master/allocator/mesos/hierarchical.cpp: patch does not apply
```
- Mesos Reviewbot Windows
On Feb. 8, 2019, 11:32 a.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69890/
> -----------------------------------------------------------
>
> (Updated Feb. 8, 2019, 11:32 a.m.)
>
>
> Review request for mesos, Benjamin Mahler and Meng Zhu.
>
>
> Bugs: MESOS-9523
> https://issues.apache.org/jira/browse/MESOS-9523
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds tests for the behavior of per-framework, per-role
> minimal allocatable resources. We validate the behavior of framework
> minimal allocatable resources below the globally configured limits and
> the behavior of empty filters or minimal resource quantities set up for
> a framework.
>
>
> Diffs
> -----
>
> src/tests/hierarchical_allocator_tests.cpp cc88afbad1b4e6bf707cb13b50c964aa01f9a3ee
>
>
> Diff: https://reviews.apache.org/r/69890/diff/2/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 69890: Added test for per-framework,
per-role minimal allocatable resources.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69890/#review212785
-----------------------------------------------------------
Patch looks great!
Reviews applied: [69900, 69902, 69818, 69862, 69889, 69821, 69890]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose --disable-libtool-wrappers --disable-parallel-test-execution' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On Feb. 11, 2019, 7:46 a.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69890/
> -----------------------------------------------------------
>
> (Updated Feb. 11, 2019, 7:46 a.m.)
>
>
> Review request for mesos, Benjamin Mahler and Meng Zhu.
>
>
> Bugs: MESOS-9523
> https://issues.apache.org/jira/browse/MESOS-9523
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds tests for the behavior of per-framework, per-role
> minimal allocatable resources. We validate the behavior of framework
> minimal allocatable resources below the globally configured limits and
> the behavior of empty filters or minimal resource quantities set up for
> a framework.
>
>
> Diffs
> -----
>
> src/tests/hierarchical_allocator_tests.cpp cc88afbad1b4e6bf707cb13b50c964aa01f9a3ee
>
>
> Diff: https://reviews.apache.org/r/69890/diff/3/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 69890: Added test for per-framework,
per-role minimal allocatable resources.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69890/#review212702
-----------------------------------------------------------
PASS: Mesos patch 69890 was successfully built and tested.
Reviews applied: `['69900', '69902', '69818', '69862', '69889', '69821', '69890']`
All the build artifacts available at: http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2871/mesos-review-69890
- Mesos Reviewbot Windows
On Feb. 11, 2019, 7:46 a.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69890/
> -----------------------------------------------------------
>
> (Updated Feb. 11, 2019, 7:46 a.m.)
>
>
> Review request for mesos, Benjamin Mahler and Meng Zhu.
>
>
> Bugs: MESOS-9523
> https://issues.apache.org/jira/browse/MESOS-9523
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds tests for the behavior of per-framework, per-role
> minimal allocatable resources. We validate the behavior of framework
> minimal allocatable resources below the globally configured limits and
> the behavior of empty filters or minimal resource quantities set up for
> a framework.
>
>
> Diffs
> -----
>
> src/tests/hierarchical_allocator_tests.cpp cc88afbad1b4e6bf707cb13b50c964aa01f9a3ee
>
>
> Diff: https://reviews.apache.org/r/69890/diff/3/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 69890: Added test for per-framework,
per-role minimal allocatable resources.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69890/#review212820
-----------------------------------------------------------
Patch looks great!
Reviews applied: [69900, 69902, 69818, 69862, 69889, 69821, 69890]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose --disable-libtool-wrappers --disable-parallel-test-execution' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On Feb. 13, 2019, 10:31 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69890/
> -----------------------------------------------------------
>
> (Updated Feb. 13, 2019, 10:31 p.m.)
>
>
> Review request for mesos, Benjamin Mahler and Meng Zhu.
>
>
> Bugs: MESOS-9523
> https://issues.apache.org/jira/browse/MESOS-9523
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds tests for the behavior of per-framework, per-role
> minimal allocatable resources. We validate the behavior of framework
> minimal allocatable resources below the globally configured limits and
> the behavior of empty filters or minimal resource quantities set up for
> a framework.
>
>
> Diffs
> -----
>
> src/tests/hierarchical_allocator_tests.cpp 1366cc51332484d7ff3b5e5b7ba767efb97172c0
>
>
> Diff: https://reviews.apache.org/r/69890/diff/4/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 69890: Added test for per-framework,
per-role minimal allocatable resources.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69890/#review212811
-----------------------------------------------------------
PASS: Mesos patch 69890 was successfully built and tested.
Reviews applied: `['69818', '69862', '69889', '69821', '69890']`
All the build artifacts available at: http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2885/mesos-review-69890
- Mesos Reviewbot Windows
On Feb. 13, 2019, 2:31 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69890/
> -----------------------------------------------------------
>
> (Updated Feb. 13, 2019, 2:31 p.m.)
>
>
> Review request for mesos, Benjamin Mahler and Meng Zhu.
>
>
> Bugs: MESOS-9523
> https://issues.apache.org/jira/browse/MESOS-9523
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds tests for the behavior of per-framework, per-role
> minimal allocatable resources. We validate the behavior of framework
> minimal allocatable resources below the globally configured limits and
> the behavior of empty filters or minimal resource quantities set up for
> a framework.
>
>
> Diffs
> -----
>
> src/tests/hierarchical_allocator_tests.cpp 1366cc51332484d7ff3b5e5b7ba767efb97172c0
>
>
> Diff: https://reviews.apache.org/r/69890/diff/4/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 69890: Added test for per-framework,
per-role minimal allocatable resources.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69890/#review212842
-----------------------------------------------------------
Ship it!
Ship It!
- Benjamin Mahler
On Feb. 14, 2019, 8:48 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69890/
> -----------------------------------------------------------
>
> (Updated Feb. 14, 2019, 8:48 p.m.)
>
>
> Review request for mesos, Benjamin Mahler and Meng Zhu.
>
>
> Bugs: MESOS-9523
> https://issues.apache.org/jira/browse/MESOS-9523
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds tests for the behavior of per-framework, per-role
> minimal allocatable resources. We validate the behavior of framework
> minimal allocatable resources below the globally configured limits and
> the behavior of empty filters or minimal resource quantities set up for
> a framework.
>
>
> Diffs
> -----
>
> src/tests/hierarchical_allocator_tests.cpp 1366cc51332484d7ff3b5e5b7ba767efb97172c0
>
>
> Diff: https://reviews.apache.org/r/69890/diff/5/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 69890: Added test for per-framework,
per-role minimal allocatable resources.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69890/#review212847
-----------------------------------------------------------
PASS: Mesos patch 69890 was successfully built and tested.
Reviews applied: `['69818', '69862', '69889', '69821', '69890']`
All the build artifacts available at: http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2890/mesos-review-69890
- Mesos Reviewbot Windows
On Feb. 14, 2019, 8:48 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69890/
> -----------------------------------------------------------
>
> (Updated Feb. 14, 2019, 8:48 p.m.)
>
>
> Review request for mesos, Benjamin Mahler and Meng Zhu.
>
>
> Bugs: MESOS-9523
> https://issues.apache.org/jira/browse/MESOS-9523
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds tests for the behavior of per-framework, per-role
> minimal allocatable resources. We validate the behavior of framework
> minimal allocatable resources below the globally configured limits and
> the behavior of empty filters or minimal resource quantities set up for
> a framework.
>
>
> Diffs
> -----
>
> src/tests/hierarchical_allocator_tests.cpp 1366cc51332484d7ff3b5e5b7ba767efb97172c0
>
>
> Diff: https://reviews.apache.org/r/69890/diff/5/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 69890: Added test for per-framework,
per-role minimal allocatable resources.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69890/
-----------------------------------------------------------
(Updated Feb. 14, 2019, 9:48 p.m.)
Review request for mesos, Benjamin Mahler and Meng Zhu.
Changes
-------
Allow empty sets of min allocatable resources
Bugs: MESOS-9523
https://issues.apache.org/jira/browse/MESOS-9523
Repository: mesos
Description
-------
This patch adds tests for the behavior of per-framework, per-role
minimal allocatable resources. We validate the behavior of framework
minimal allocatable resources below the globally configured limits and
the behavior of empty filters or minimal resource quantities set up for
a framework.
Diffs (updated)
-----
src/tests/hierarchical_allocator_tests.cpp 1366cc51332484d7ff3b5e5b7ba767efb97172c0
Diff: https://reviews.apache.org/r/69890/diff/5/
Changes: https://reviews.apache.org/r/69890/diff/4-5/
Testing
-------
`make check`
Thanks,
Benjamin Bannier
Re: Review Request 69890: Added test for per-framework,
per-role minimal allocatable resources.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69890/
-----------------------------------------------------------
(Updated Feb. 13, 2019, 11:31 p.m.)
Review request for mesos, Benjamin Mahler and Meng Zhu.
Changes
-------
Remove test cases which are not needed anymore
Bugs: MESOS-9523
https://issues.apache.org/jira/browse/MESOS-9523
Repository: mesos
Description
-------
This patch adds tests for the behavior of per-framework, per-role
minimal allocatable resources. We validate the behavior of framework
minimal allocatable resources below the globally configured limits and
the behavior of empty filters or minimal resource quantities set up for
a framework.
Diffs (updated)
-----
src/tests/hierarchical_allocator_tests.cpp 1366cc51332484d7ff3b5e5b7ba767efb97172c0
Diff: https://reviews.apache.org/r/69890/diff/4/
Changes: https://reviews.apache.org/r/69890/diff/3-4/
Testing
-------
`make check`
Thanks,
Benjamin Bannier
Re: Review Request 69890: Added test for per-framework,
per-role minimal allocatable resources.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69890/
-----------------------------------------------------------
(Updated Feb. 11, 2019, 4:46 p.m.)
Review request for mesos, Benjamin Mahler and Meng Zhu.
Changes
-------
Fix comments as suggested by Ben
Bugs: MESOS-9523
https://issues.apache.org/jira/browse/MESOS-9523
Repository: mesos
Description
-------
This patch adds tests for the behavior of per-framework, per-role
minimal allocatable resources. We validate the behavior of framework
minimal allocatable resources below the globally configured limits and
the behavior of empty filters or minimal resource quantities set up for
a framework.
Diffs (updated)
-----
src/tests/hierarchical_allocator_tests.cpp cc88afbad1b4e6bf707cb13b50c964aa01f9a3ee
Diff: https://reviews.apache.org/r/69890/diff/3/
Changes: https://reviews.apache.org/r/69890/diff/2-3/
Testing
-------
`make check`
Thanks,
Benjamin Bannier
Re: Review Request 69890: Added test for per-framework,
per-role minimal allocatable resources.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69890/
-----------------------------------------------------------
(Updated Feb. 8, 2019, 12:32 p.m.)
Review request for mesos, Benjamin Mahler and Meng Zhu.
Changes
-------
Address comments from Ben
Bugs: MESOS-9523
https://issues.apache.org/jira/browse/MESOS-9523
Repository: mesos
Description
-------
This patch adds tests for the behavior of per-framework, per-role
minimal allocatable resources. We validate the behavior of framework
minimal allocatable resources below the globally configured limits and
the behavior of empty filters or minimal resource quantities set up for
a framework.
Diffs (updated)
-----
src/tests/hierarchical_allocator_tests.cpp cc88afbad1b4e6bf707cb13b50c964aa01f9a3ee
Diff: https://reviews.apache.org/r/69890/diff/2/
Changes: https://reviews.apache.org/r/69890/diff/1-2/
Testing
-------
`make check`
Thanks,
Benjamin Bannier
Re: Review Request 69890: Added test for per-framework,
per-role minimal allocatable resources.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On Feb. 5, 2019, 8:03 p.m., Benjamin Mahler wrote:
> > src/tests/hierarchical_allocator_tests.cpp
> > Lines 2287 (patched)
> > <https://reviews.apache.org/r/69890/diff/1/?file=2123899#file2123899line2287>
> >
> > any reason not to stick to using "minimum" for these? ditto below
> >
> > is "finite" needed here?
Fixed.
> On Feb. 5, 2019, 8:03 p.m., Benjamin Mahler wrote:
> > src/tests/hierarchical_allocator_tests.cpp
> > Lines 2287-2288 (patched)
> > <https://reviews.apache.org/r/69890/diff/1/?file=2123899#file2123899line2287>
> >
> > suggestion:
> >
> > "resources, it correctly overrides the global minimum. We check this by ensuring it gets an offer when the global minimum is not satisfied but the framework's role specific mininium is"
Fixed.
> On Feb. 5, 2019, 8:03 p.m., Benjamin Mahler wrote:
> > src/tests/hierarchical_allocator_tests.cpp
> > Lines 2352-2353 (patched)
> > <https://reviews.apache.org/r/69890/diff/1/?file=2123899#file2123899line2352>
> >
> > Do we actually need these two lines? It seems like having a "set" but empty `min_allocatable_resources` should be enough? (i.e. without having to add a single array item within its `quantities`)
Yes, I believe this is needed here. I fixed the test names like pointed out by you below.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69890/#review212563
-----------------------------------------------------------
On Feb. 8, 2019, 12:32 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69890/
> -----------------------------------------------------------
>
> (Updated Feb. 8, 2019, 12:32 p.m.)
>
>
> Review request for mesos, Benjamin Mahler and Meng Zhu.
>
>
> Bugs: MESOS-9523
> https://issues.apache.org/jira/browse/MESOS-9523
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds tests for the behavior of per-framework, per-role
> minimal allocatable resources. We validate the behavior of framework
> minimal allocatable resources below the globally configured limits and
> the behavior of empty filters or minimal resource quantities set up for
> a framework.
>
>
> Diffs
> -----
>
> src/tests/hierarchical_allocator_tests.cpp cc88afbad1b4e6bf707cb13b50c964aa01f9a3ee
>
>
> Diff: https://reviews.apache.org/r/69890/diff/2/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 69890: Added test for per-framework,
per-role minimal allocatable resources.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69890/#review212563
-----------------------------------------------------------
Looks good! Just some minor comments below
src/tests/hierarchical_allocator_tests.cpp
Lines 2287 (patched)
<https://reviews.apache.org/r/69890/#comment298386>
any reason not to stick to using "minimum" for these? ditto below
is "finite" needed here?
src/tests/hierarchical_allocator_tests.cpp
Lines 2287-2288 (patched)
<https://reviews.apache.org/r/69890/#comment298387>
suggestion:
"resources, it correctly overrides the global minimum. We check this by ensuring it gets an offer when the global minimum is not satisfied but the framework's role specific mininium is"
src/tests/hierarchical_allocator_tests.cpp
Lines 2296 (patched)
<https://reviews.apache.org/r/69890/#comment298385>
Can we pass a master::Flags object into initialize that has an explicit min allocatable resources flag set? (It seems it already is set up to take this flag)
Right now this test seems to be making a pretty strong assumption that MIN_CPUS is what's being used?
Ditto for the other tests below
src/tests/hierarchical_allocator_tests.cpp
Lines 2345-2346 (patched)
<https://reviews.apache.org/r/69890/#comment298391>
seems like this should describe that "a single empty quantity" is used?
src/tests/hierarchical_allocator_tests.cpp
Lines 2352-2353 (patched)
<https://reviews.apache.org/r/69890/#comment298388>
Do we actually need these two lines? It seems like having a "set" but empty `min_allocatable_resources` should be enough? (i.e. without having to add a single array item within its `quantities`)
src/tests/hierarchical_allocator_tests.cpp
Lines 2357 (patched)
<https://reviews.apache.org/r/69890/#comment298389>
Ditto earlier comment, it's probably not helpful for the reader to see a mixture of "minimal" and "minimum" used?
src/tests/hierarchical_allocator_tests.cpp
Lines 2378-2381 (patched)
<https://reviews.apache.org/r/69890/#comment298390>
This seems like the "EmptyMinAllocatable" case and the one above seems like the "EmptyQuantityMinAllocatable" case?
- Benjamin Mahler
On Feb. 4, 2019, 10:05 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69890/
> -----------------------------------------------------------
>
> (Updated Feb. 4, 2019, 10:05 p.m.)
>
>
> Review request for mesos, Benjamin Mahler and Meng Zhu.
>
>
> Bugs: MESOS-9523
> https://issues.apache.org/jira/browse/MESOS-9523
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds tests for the behavior of per-framework, per-role
> minimal allocatable resources. We validate the behavior of framework
> minimal allocatable resources below the globally configured limits and
> the behavior of empty filters or minimal resource quantities set up for
> a framework.
>
>
> Diffs
> -----
>
> src/tests/hierarchical_allocator_tests.cpp cc88afbad1b4e6bf707cb13b50c964aa01f9a3ee
>
>
> Diff: https://reviews.apache.org/r/69890/diff/1/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 69890: Added test for per-framework,
per-role minimal allocatable resources.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69890/#review212541
-----------------------------------------------------------
PASS: Mesos patch 69890 was successfully built and tested.
Reviews applied: `['69818', '69862', '69889', '69821', '69890']`
All the build artifacts available at: http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2849/mesos-review-69890
- Mesos Reviewbot Windows
On Feb. 4, 2019, 10:05 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69890/
> -----------------------------------------------------------
>
> (Updated Feb. 4, 2019, 10:05 p.m.)
>
>
> Review request for mesos, Benjamin Mahler and Meng Zhu.
>
>
> Bugs: MESOS-9523
> https://issues.apache.org/jira/browse/MESOS-9523
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds tests for the behavior of per-framework, per-role
> minimal allocatable resources. We validate the behavior of framework
> minimal allocatable resources below the globally configured limits and
> the behavior of empty filters or minimal resource quantities set up for
> a framework.
>
>
> Diffs
> -----
>
> src/tests/hierarchical_allocator_tests.cpp cc88afbad1b4e6bf707cb13b50c964aa01f9a3ee
>
>
> Diff: https://reviews.apache.org/r/69890/diff/1/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>