Re: Review Request 69890: Added test for per-framework, per-role minimal allocatable resources.

[Mesos Reviewbot Windows]

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

[Benjamin Bannier]

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

[Benjamin Mahler]

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

[Benjamin Bannier]

> On Feb. 8, 2019, 9:47 p.m., Benjamin Mahler wrote:
> > src/tests/hierarchical_allocator_tests.cpp
> > Lines 2357-2374 (patched)
> > 
> >
> > 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.

[Mesos Reviewbot]

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

[Mesos Reviewbot Windows]

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

[Benjamin Bannier]

> On Feb. 8, 2019, 9:47 p.m., Benjamin Mahler wrote:
> > src/tests/hierarchical_allocator_tests.cpp
> > Lines 2357-2374 (patched)
> > 
> >
> > 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.

[Benjamin Bannier]

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

[Mesos Reviewbot]

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

[Benjamin Mahler]

> On Feb. 8, 2019, 8:47 p.m., Benjamin Mahler wrote:
> > src/tests/hierarchical_allocator_tests.cpp
> > Lines 2357-2374 (patched)
> > 
> >
> > 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.

[Benjamin Bannier]

> On Feb. 8, 2019, 9:47 p.m., Benjamin Mahler wrote:
> > src/tests/hierarchical_allocator_tests.cpp
> > Lines 2357-2374 (patched)
> > 
> >
> > 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.

[Mesos Reviewbot Windows]

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

[Benjamin Bannier]

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

[Benjamin Mahler]

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


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.

[Mesos Reviewbot Windows]

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

[Benjamin Bannier]

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

[Benjamin Bannier]

> On Feb. 5, 2019, 8:03 p.m., Benjamin Mahler wrote:
> > src/tests/hierarchical_allocator_tests.cpp
> > Lines 2287 (patched)
> > 
> >
> > 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)
> > 
> >
> > 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)
> > 
> >
> > 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.

[Benjamin Mahler]

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


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)


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)


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)


seems like this should describe that "a single empty quantity" is used?



src/tests/hierarchical_allocator_tests.cpp
Lines 2352-2353 (patched)


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)


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)


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.

[Mesos Reviewbot Windows]

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

Review Request 69890: Added test for per-framework, per-role minimal allocatable resources.

[Benjamin Bannier]

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