Re: Review Request 69876: Removed operations from master state when an agent is downgraded.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 69876 was successfully built and tested.

Reviews applied: `['69876']`

All the build artifacts available at: 
http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2842/mesos-review-69876

- Mesos Reviewbot Windows


On Jan. 31, 2019, 11:08 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69876/
> ---
> 
> (Updated Jan. 31, 2019, 11:08 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Chun-Hung Hsiao, and Gastón 
> Kleiman.
> 
> 
> Bugs: MESOS-9535
> https://issues.apache.org/jira/browse/MESOS-9535
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When an agent is downgraded from one with the AGENT_OPERATION_FEEDBACK
> capability to one without this capability, the master needs to remove
> terminal-but-unACKed operations from its state which operate on agent
> default resources, since the downgraded agent will not resend status
> updates for these operations.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp f74b7c280569e1c24e0940463bb28bd795d429d5 
>   src/tests/master_tests.cpp acc6096239e4992bdca084d0d644ab4a2385 
> 
> 
> Diff: https://reviews.apache.org/r/69876/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> `bin/mesos-tests.sh --gtest_filter="*CleanupOperationsAfterAgentDowngrade*" 
> --gtest_repeat=-1 --gtest_break_on_failure`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Review Request 69876: Removed operations from master state when an agent is downgraded.

[Greg Mann]

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

Review request for mesos, Benjamin Bannier, Chun-Hung Hsiao, and Gastón Kleiman.


Bugs: MESOS-9535
https://issues.apache.org/jira/browse/MESOS-9535


Repository: mesos


Description
---

When an agent is downgraded from one with the AGENT_OPERATION_FEEDBACK
capability to one without this capability, the master needs to remove
terminal-but-unACKed operations from its state which operate on agent
default resources, since the downgraded agent will not resend status
updates for these operations.


Diffs
-

  src/master/master.cpp f74b7c280569e1c24e0940463bb28bd795d429d5 
  src/tests/master_tests.cpp acc6096239e4992bdca084d0d644ab4a2385 


Diff: https://reviews.apache.org/r/69876/diff/1/


Testing
---

`make check`
`bin/mesos-tests.sh --gtest_filter="*CleanupOperationsAfterAgentDowngrade*" 
--gtest_repeat=-1 --gtest_break_on_failure`


Thanks,

Greg Mann

Re: Review Request 69862: Validated static framework offer filters.

[Benjamin Mahler]

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


Fix it, then Ship it!





src/common/validation.cpp
Lines 629-632 (patched)


Should we also disallow NaN and +inf? (along with tests for these cases?)


- Benjamin Mahler


On Jan. 31, 2019, 3:15 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69862/
> ---
> 
> (Updated Jan. 31, 2019, 3:15 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 validation to framework-specified static offer filters.
> In particular we enforce that minimal resource quantities cannot be
> negative.
> 
> 
> Diffs
> -
> 
>   src/common/validation.hpp 5929ac24fa7d7d2f7f08bc30c5071716e6cf46cb 
>   src/common/validation.cpp 4a64fc8ac83fd741eb2cbc70772322b347dbcb4e 
>   src/master/validation.hpp a5bdff6a9301631dac9970568a1346460939c861 
>   src/master/validation.cpp 7e688bb657cd44f98f082b5efe7178edcb7ce595 
>   src/tests/common_validation_tests.cpp 
> 97466ab38f3156155655520c81b17cc6ec2cd779 
>   src/tests/master_validation_tests.cpp 
> c00e8bb315c28bdf438da2089dd81f5e348982e5 
> 
> 
> Diff: https://reviews.apache.org/r/69862/diff/2/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 69821: Enforced minimal allocatable resources in the hierarchical allocator.

[Benjamin Mahler]

> On Jan. 29, 2019, 8:37 p.m., Benjamin Mahler wrote:
> > src/tests/hierarchical_allocator_tests.cpp
> > Lines 2336 (patched)
> > 
> >
> > FrameworkEmptyMinAllocatable
> 
> Benjamin Bannier wrote:
> 

This and the one above are naming suggestions


- Benjamin


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


On Jan. 31, 2019, 3:16 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69821/
> ---
> 
> (Updated Jan. 31, 2019, 3:16 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 modifies the hierarchical allocator to take
> framework-specified minimal allocatable resources into account.
> 
> While previously the allocator was inspecting the minimal allocatable
> resources specified in its global options, it can now also inspects
> framework-specific resource requirements. With that frameworks can e.g.,
> configure resource requirements above the default minimal allocatable
> resource, or opt into receiving resources considered too small to be
> allocatable by the allocator in its default behavior.
> 
> For that we change the hierarchical allocator's `allocatable` function
> to be framework and role-specific. As that does in some places not allow
> us to abort iterating over candidate resource consumers like before an
> additional check on whether any resources are left in an allocation
> cycle is added as a break condition.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> 1420c2638786d85f7b04379e5d79e59990c3e6cf 
>   src/master/allocator/mesos/hierarchical.cpp 
> bb9a9c95979f36c0564af5b3babb1c43077a363b 
>   src/tests/hierarchical_allocator_tests.cpp 
> cc88afbad1b4e6bf707cb13b50c964aa01f9a3ee 
> 
> 
> Diff: https://reviews.apache.org/r/69821/diff/4/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> File Attachments
> 
> 
> Ratio new/old timings
>   
> https://reviews.apache.org/media/uploaded/files/2019/01/31/d76189de-8882-4aff-956b-090dab729358__new_over_old.png
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 69818: Added offer filters to static framework configuration.

[Benjamin Mahler]

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




src/master/framework.cpp
Lines 504 (patched)


Can you guard this with a has check?

As it stands it will produce a different message if newInfo has it unset 
(info will have it set vs newInfo has it not set)


- Benjamin Mahler


On Jan. 31, 2019, 3:29 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69818/
> ---
> 
> (Updated Jan. 31, 2019, 3:29 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 a new field to `FrameworkInfo` which can be used by
> frameworks to configure static, per-role resource requirements. We
> currently support specifying minimal allocatable resources which can
> e.g., be interpreted by allocators to filter out resources not relevant
> to frameworks, or make allocations below globally configured minimal
> allocatable resources for particular frameworks.
> 
> We use a new message `ResourceQuantities` to specify resource
> requirements. This class is modelled after the internal class of the
> same name, and we provide a conversion from the API to the internal
> type.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 9d5c2b53164adb44eb4a6f12a507e009bd607940 
>   include/mesos/v1/mesos.proto aef31eb5b8781182d3f42d935b12470b319027ed 
>   src/common/resource_quantities.hpp 11eb426104577b7977c2307df3e4917085cd 
>   src/common/resource_quantities.cpp 320983929cd7d14973c4b98d6ed5338de690ff5f 
>   src/master/framework.cpp 4089cf4dfc65ac4dc5a092c04f4c2022ed8a1587 
> 
> 
> Diff: https://reviews.apache.org/r/69818/diff/6/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 69871: Added more documentation for operation feedback.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 69871 was successfully built and tested.

Reviews applied: `['69871']`

All the build artifacts available at: 
http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2841/mesos-review-69871

- Mesos Reviewbot Windows


On Jan. 31, 2019, 5:57 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69871/
> ---
> 
> (Updated Jan. 31, 2019, 5:57 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Gastón Kleiman, and James 
> DeFelice.
> 
> 
> Bugs: MESOS-9477
> https://issues.apache.org/jira/browse/MESOS-9477
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added more documentation for operation feedback.
> 
> 
> Diffs
> -
> 
>   docs/high-availability-framework-guide.md 
> 1bf58e166c76af9ef9d7182d9d4e413bc02e253c 
>   docs/reconciliation.md bfb117c11969f97127ef2c55321191232814a1c2 
>   docs/scheduler-http-api.md 84f6187943b60d4472a4b7e0c667678ab055e482 
> 
> 
> Diff: https://reviews.apache.org/r/69871/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 69680: Have master acknowledge operation updates of completed frameworks.

[Greg Mann]

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


Fix it, then Ship it!





src/tests/master_tests.cpp
Line 9331 (original), 9334 (patched)


s/for that//


- Greg Mann


On Jan. 29, 2019, 1:21 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69680/
> ---
> 
> (Updated Jan. 29, 2019, 1:21 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Greg Mann.
> 
> 
> Bugs: MESOS-9434
> https://issues.apache.org/jira/browse/MESOS-9434
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> After a framework was removed and has unacknowledged operations status
> updates, it was impossible to remove terminal operations as nobody could
> acknowledge them.
> 
> In this patch we make the master acknowledge operation status updates
> for frameworks it knows are removed so that e.g., terminal operations
> can be removed. Since masters do not persist completed frameworks this
> is not reliable (e.g., an agent was partitioned for a long time and
> still tracks a completed framework's `FrameworkInfo`, and comes back
> only after the master knowing about the framework's completion has
> failed over). We merely extend the existing master behavior (e.g., send
> `ShutdownFrameworkMessage` to all currently registered agents) to
> operations.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 2e0a0de4d745112011370741b8cedbb6db17b915 
>   src/tests/master_tests.cpp 1f8da63aa313c4679c0d7a3934802af1474e8f28 
> 
> 
> Diff: https://reviews.apache.org/r/69680/diff/2/
> 
> 
> Testing
> ---
> 
> * `make check`
> * tested on a number of configurations in internal CI
> * ran added test in repetition, both with and without additional stress
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 69871: Added more documentation for operation feedback.

[Greg Mann]

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

(Updated Jan. 31, 2019, 5:57 p.m.)


Review request for mesos, Benjamin Bannier, Gastón Kleiman, and James DeFelice.


Bugs: MESOS-9477
https://issues.apache.org/jira/browse/MESOS-9477


Repository: mesos


Description
---

Added more documentation for operation feedback.


Diffs (updated)
-

  docs/high-availability-framework-guide.md 
1bf58e166c76af9ef9d7182d9d4e413bc02e253c 
  docs/reconciliation.md bfb117c11969f97127ef2c55321191232814a1c2 
  docs/scheduler-http-api.md 84f6187943b60d4472a4b7e0c667678ab055e482 


Diff: https://reviews.apache.org/r/69871/diff/2/

Changes: https://reviews.apache.org/r/69871/diff/1-2/


Testing
---


Thanks,

Greg Mann

Re: Review Request 69871: Added more documentation for operation feedback.

[Greg Mann]

> On Jan. 31, 2019, 2:52 a.m., James DeFelice wrote:
> > docs/high-availability-framework-guide.md
> > Lines 254 (patched)
> > 
> >
> > What about OPERATION_UNKNOWN, where does that fit in here?

Whoops, I definitely intended to include it! Thanks!


- Greg


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


On Jan. 31, 2019, 2:11 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69871/
> ---
> 
> (Updated Jan. 31, 2019, 2:11 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Gastón Kleiman, and James 
> DeFelice.
> 
> 
> Bugs: MESOS-9477
> https://issues.apache.org/jira/browse/MESOS-9477
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added more documentation for operation feedback.
> 
> 
> Diffs
> -
> 
>   docs/high-availability-framework-guide.md 
> 1bf58e166c76af9ef9d7182d9d4e413bc02e253c 
>   docs/reconciliation.md bfb117c11969f97127ef2c55321191232814a1c2 
>   docs/scheduler-http-api.md 84f6187943b60d4472a4b7e0c667678ab055e482 
> 
> 
> Diff: https://reviews.apache.org/r/69871/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 69821: Enforced minimal allocatable resources in the hierarchical allocator.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 69821 was successfully built and tested.

Reviews applied: `['69818', '69862', '69821']`

All the build artifacts available at: 
http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2840/mesos-review-69821

- Mesos Reviewbot Windows


On Jan. 31, 2019, 3:16 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69821/
> ---
> 
> (Updated Jan. 31, 2019, 3:16 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 modifies the hierarchical allocator to take
> framework-specified minimal allocatable resources into account.
> 
> While previously the allocator was inspecting the minimal allocatable
> resources specified in its global options, it can now also inspects
> framework-specific resource requirements. With that frameworks can e.g.,
> configure resource requirements above the default minimal allocatable
> resource, or opt into receiving resources considered too small to be
> allocatable by the allocator in its default behavior.
> 
> For that we change the hierarchical allocator's `allocatable` function
> to be framework and role-specific. As that does in some places not allow
> us to abort iterating over candidate resource consumers like before an
> additional check on whether any resources are left in an allocation
> cycle is added as a break condition.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> 1420c2638786d85f7b04379e5d79e59990c3e6cf 
>   src/master/allocator/mesos/hierarchical.cpp 
> bb9a9c95979f36c0564af5b3babb1c43077a363b 
>   src/tests/hierarchical_allocator_tests.cpp 
> cc88afbad1b4e6bf707cb13b50c964aa01f9a3ee 
> 
> 
> Diff: https://reviews.apache.org/r/69821/diff/4/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> File Attachments
> 
> 
> Ratio new/old timings
>   
> https://reviews.apache.org/media/uploaded/files/2019/01/31/d76189de-8882-4aff-956b-090dab729358__new_over_old.png
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 69818: Added offer filters to static framework configuration.

[Benjamin Bannier]

> On Jan. 29, 2019, 9:14 p.m., Benjamin Mahler wrote:
> > I think you missed my top level comment from the last review to add context 
> > to the commit about the min allocatable flag and how this is providing a 
> > per-framework/role override of that global flag?
> > 
> > Also, we're missing a patch for the master to allow changing the value upon 
> > framework re-registration?

Re:commit message, see my comment here, 
https://reviews.apache.org/r/69818/#comment298165.

Re:changing the offer_filters, updated this patch.


- Benjamin


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


On Jan. 31, 2019, 4:29 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69818/
> ---
> 
> (Updated Jan. 31, 2019, 4:29 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 a new field to `FrameworkInfo` which can be used by
> frameworks to configure static, per-role resource requirements. We
> currently support specifying minimal allocatable resources which can
> e.g., be interpreted by allocators to filter out resources not relevant
> to frameworks, or make allocations below globally configured minimal
> allocatable resources for particular frameworks.
> 
> We use a new message `ResourceQuantities` to specify resource
> requirements. This class is modelled after the internal class of the
> same name, and we provide a conversion from the API to the internal
> type.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 9d5c2b53164adb44eb4a6f12a507e009bd607940 
>   include/mesos/v1/mesos.proto aef31eb5b8781182d3f42d935b12470b319027ed 
>   src/common/resource_quantities.hpp 11eb426104577b7977c2307df3e4917085cd 
>   src/common/resource_quantities.cpp 320983929cd7d14973c4b98d6ed5338de690ff5f 
>   src/master/framework.cpp 4089cf4dfc65ac4dc5a092c04f4c2022ed8a1587 
> 
> 
> Diff: https://reviews.apache.org/r/69818/diff/6/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 69818: Added offer filters to static framework configuration.

[Benjamin Bannier]

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

(Updated Jan. 31, 2019, 4:29 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 a new field to `FrameworkInfo` which can be used by
frameworks to configure static, per-role resource requirements. We
currently support specifying minimal allocatable resources which can
e.g., be interpreted by allocators to filter out resources not relevant
to frameworks, or make allocations below globally configured minimal
allocatable resources for particular frameworks.

We use a new message `ResourceQuantities` to specify resource
requirements. This class is modelled after the internal class of the
same name, and we provide a conversion from the API to the internal
type.


Diffs (updated)
-

  include/mesos/mesos.proto 9d5c2b53164adb44eb4a6f12a507e009bd607940 
  include/mesos/v1/mesos.proto aef31eb5b8781182d3f42d935b12470b319027ed 
  src/common/resource_quantities.hpp 11eb426104577b7977c2307df3e4917085cd 
  src/common/resource_quantities.cpp 320983929cd7d14973c4b98d6ed5338de690ff5f 
  src/master/framework.cpp 4089cf4dfc65ac4dc5a092c04f4c2022ed8a1587 


Diff: https://reviews.apache.org/r/69818/diff/6/

Changes: https://reviews.apache.org/r/69818/diff/5-6/


Testing
---

`make check`


Thanks,

Benjamin Bannier

Re: Review Request 69821: Enforced minimal allocatable resources in the hierarchical allocator.

[Benjamin Bannier]

> On Jan. 29, 2019, 9:37 p.m., Benjamin Mahler wrote:
> > The commit description makes it sound like the filter is using the 
> > **union** of the flag and the per-framework/role override:
> > 
> > > This set is populated with any minimal allocatable resources specified in 
> > > the allocator's options and any framework's minimal allocatable resources.
> > 
> > However, it should be an override (which is what the patch correctly does 
> > AFAICT). Probably, just using "override" in the explanation would make this 
> > very clear to the reader.
> > 
> > IMO, we only really need the first paragraph of the description:
> > 
> > > This patch modifies the hierarchical allocator to take 
> > > framework-specified minimal allocatable resources into account.
> > 
> > We could make it a bit clearer:
> > 
> > > This patch modifies the allocator to take the framework-specified minimum 
> > > allocatable resources into account. These act as an override of the 
> > > existing `min_allocatable_resources` flag.
> > 
> > What more needs to be said?

Sorry for not updating the commit message when posting my WIP result; updated 
now.


> On Jan. 29, 2019, 9:37 p.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp
> > Lines 1952-1968 (original), 1978-1987 (patched)
> > 
> >
> > This makes it look like we should consolidate into the `isFiltered` 
> > function?

See below.


> On Jan. 29, 2019, 9:37 p.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp
> > Line 2069 (original), 2082 (patched)
> > 
> >
> > Now that this is a continue instead of a break, it seems they can be 
> > consolidated into 1 below?
> > 
> > Also, can you post the benchmark numbers before and after this patch to 
> > make sure we're not accidentally regressing pretty badly on performance?

Let's keep this separated from the filtering step, if only to be able to 
document it more explicitly.

After our offline discussion I added an additional check just before this one 
which triggers a `continue` if `toAllocate.empty()`. This does prevent some 
useless work.

As a benchmark I ran 
`SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.DeclineOffers/4` 
(1000 agents, 500 frameworks). I patched the test to only execute 
`num_frameworks` allocation rounds, and also printed timings in `ms` instead of 
pretty-printing. The logs from an optimized build are:

1) `9f6ccbd41a55846e54297ecb31fddbeee3be50c9`
```
[ RUN  ] 
SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.DeclineOffers/4
Using 1000 agents and 500 frameworks
Added 500 frameworks in 15.6022ms
Added 1000 agents in 82.2129ms
round 0 allocate() took 43.379ms to make 1000 offers after filtering 1000 offers
round 1 allocate() took 44.7003ms to make 1000 offers after filtering 2000 
offers
round 2 allocate() took 40.1724ms to make 1000 offers after filtering 3000 
offers
round 3 allocate() took 56.499ms to make 1000 offers after filtering 4000 offers
round 4 allocate() took 42.4679ms to make 1000 offers after filtering 5000 
offers
round 5 allocate() took 43.2156ms to make 1000 offers after filtering 6000 
offers
round 6 allocate() took 45.9366ms to make 1000 offers after filtering 7000 
offers
round 7 allocate() took 40.4537ms to make 1000 offers after filtering 8000 
offers
round 8 allocate() took 48.6968ms to make 1000 offers after filtering 9000 
offers
round 9 allocate() took 43.5264ms to make 1000 offers after filtering 1 
offers
round 10 allocate() took 42.1773ms to make 1000 offers after filtering 11000 
offers
round 11 allocate() took 37.3388ms to make 1000 offers after filtering 12000 
offers
round 12 allocate() took 40.3328ms to make 1000 offers after filtering 13000 
offers
round 13 allocate() took 43.9844ms to make 1000 offers after filtering 14000 
offers
round 14 allocate() took 36.8533ms to make 1000 offers after filtering 15000 
offers
round 15 allocate() took 41.1868ms to make 1000 offers after filtering 16000 
offers
round 16 allocate() took 44.1921ms to make 1000 offers after filtering 17000 
offers
round 17 allocate() took 44.6911ms to make 1000 offers after filtering 18000 
offers
round 18 allocate() took 40.2239ms to make 1000 offers after filtering 19000 
offers
round 19 allocate() took 46.5701ms to make 1000 offers after filtering 2 
offers
round 20 allocate() took 41.5735ms to make 1000 offers after filtering 21000 
offers
round 21 allocate() took 44.3456ms to make 1000 offers after filtering 22000 
offers
round 22 allocate() took 42.7894ms to make 1000 offers after filtering 23000 
offers
round 23 allocate() took 39.352ms to make 1000 offers after filtering 24000 
offers
round 24 allocate() took 40.5653ms to make 1000 offers after filtering 25000 
offers
round 25 allocate() took 46.8886ms to make 1000 offers after filtering 26000 
offers

Re: Review Request 69818: Added offer filters to static framework configuration.

[Benjamin Bannier]

> On Jan. 29, 2019, 4:09 a.m., Benjamin Mahler wrote:
> > include/mesos/mesos.proto
> > Lines 1519 (patched)
> > 
> >
> > I'm not sure if this gives much information to the reader, thoughts on 
> > the following?
> > 
> > ```
> > /**
> >  * Represents filters that allow a framework to control the
> >  * shape of offers that will be sent to its role(s). These
> >  * filters apply globally to any agent (unlike the existing
> >  * `DECLINE` filter, which is a time based resource subset
> >  * filter that only applies to the agent that was declined).
> >  */
> > ```
> 
> Benjamin Bannier wrote:
> I thought about something like that as well, but there is nothing in the 
> allocator interface enforcing it, e.g., an allocator could just ignore this 
> information.
> 
> I'd suggest to not make promises here where it isn't much more than a 
> container.
> 
> Benjamin Mahler wrote:
> From a practical standpoint, there are 0 alternative allocator 
> implementations (and that makes sense since it's not really the right 
> interface point). We already document a lot based on the assumption that an 
> allocator (i.e ours) is enforcing things like quota, drf, etc etc. So I think 
> we should just assume it and document accordingly.

Isn't it wrong to both provide an interface to use a custom allocator, and then 
make claims we cannot promise to fullfil in a public API? I believe treating 
the allocator module interface as unstable is one thing, but giving users the 
illusion that we control certain behavior a completely different one.

Suggest to keep the brief comment. Dropping.


- Benjamin


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


On Jan. 31, 2019, 4:15 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69818/
> ---
> 
> (Updated Jan. 31, 2019, 4:15 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 a new field to `FrameworkInfo` which can be used by
> frameworks to configure static, per-role resource requirements. We
> currently support specifying minimal allocatable resources which can
> e.g., be interpreted by allocators to filter out resources not relevant
> to frameworks, or make allocations below globally configured minimal
> allocatable resources for particular frameworks.
> 
> We use a new message `ResourceQuantities` to specify resource
> requirements. This class is modelled after the internal class of the
> same name, and we provide a conversion from the API to the internal
> type.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 9d5c2b53164adb44eb4a6f12a507e009bd607940 
>   include/mesos/v1/mesos.proto aef31eb5b8781182d3f42d935b12470b319027ed 
>   src/common/resource_quantities.hpp 11eb426104577b7977c2307df3e4917085cd 
>   src/common/resource_quantities.cpp 320983929cd7d14973c4b98d6ed5338de690ff5f 
> 
> 
> Diff: https://reviews.apache.org/r/69818/diff/5/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 69862: Validated static framework offer filters.

[Benjamin Bannier]

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

(Updated Jan. 31, 2019, 4:15 p.m.)


Review request for mesos, Benjamin Mahler and Meng Zhu.


Changes
---

Add more tests.


Bugs: MESOS-9523
https://issues.apache.org/jira/browse/MESOS-9523


Repository: mesos


Description (updated)
---

This patch adds validation to framework-specified static offer filters.
In particular we enforce that minimal resource quantities cannot be
negative.


Diffs (updated)
-

  src/common/validation.hpp 5929ac24fa7d7d2f7f08bc30c5071716e6cf46cb 
  src/common/validation.cpp 4a64fc8ac83fd741eb2cbc70772322b347dbcb4e 
  src/master/validation.hpp a5bdff6a9301631dac9970568a1346460939c861 
  src/master/validation.cpp 7e688bb657cd44f98f082b5efe7178edcb7ce595 
  src/tests/common_validation_tests.cpp 
97466ab38f3156155655520c81b17cc6ec2cd779 
  src/tests/master_validation_tests.cpp 
c00e8bb315c28bdf438da2089dd81f5e348982e5 


Diff: https://reviews.apache.org/r/69862/diff/2/

Changes: https://reviews.apache.org/r/69862/diff/1-2/


Testing
---

`make check`


Thanks,

Benjamin Bannier

Re: Review Request 69821: Enforced minimal allocatable resources in the hierarchical allocator.

[Benjamin Bannier]

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

(Updated Jan. 31, 2019, 4:16 p.m.)


Review request for mesos, Benjamin Mahler and Meng Zhu.


Changes
---

Addressed comments from Ben here and offline.


Bugs: MESOS-9523
https://issues.apache.org/jira/browse/MESOS-9523


Repository: mesos


Description (updated)
---

This patch modifies the hierarchical allocator to take
framework-specified minimal allocatable resources into account.

While previously the allocator was inspecting the minimal allocatable
resources specified in its global options, it can now also inspects
framework-specific resource requirements. With that frameworks can e.g.,
configure resource requirements above the default minimal allocatable
resource, or opt into receiving resources considered too small to be
allocatable by the allocator in its default behavior.

For that we change the hierarchical allocator's `allocatable` function
to be framework and role-specific. As that does in some places not allow
us to abort iterating over candidate resource consumers like before an
additional check on whether any resources are left in an allocation
cycle is added as a break condition.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.hpp 
1420c2638786d85f7b04379e5d79e59990c3e6cf 
  src/master/allocator/mesos/hierarchical.cpp 
bb9a9c95979f36c0564af5b3babb1c43077a363b 
  src/tests/hierarchical_allocator_tests.cpp 
cc88afbad1b4e6bf707cb13b50c964aa01f9a3ee 


Diff: https://reviews.apache.org/r/69821/diff/4/

Changes: https://reviews.apache.org/r/69821/diff/3-4/


Testing
---

`make check`


File Attachments (updated)


Ratio new/old timings
  
https://reviews.apache.org/media/uploaded/files/2019/01/31/d76189de-8882-4aff-956b-090dab729358__new_over_old.png


Thanks,

Benjamin Bannier

Re: Review Request 69818: Added offer filters to static framework configuration.

[Benjamin Bannier]

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

(Updated Jan. 31, 2019, 4:15 p.m.)


Review request for mesos, Benjamin Mahler and Meng Zhu.


Changes
---

Rebased.


Bugs: MESOS-9523
https://issues.apache.org/jira/browse/MESOS-9523


Repository: mesos


Description (updated)
---

This patch adds a new field to `FrameworkInfo` which can be used by
frameworks to configure static, per-role resource requirements. We
currently support specifying minimal allocatable resources which can
e.g., be interpreted by allocators to filter out resources not relevant
to frameworks, or make allocations below globally configured minimal
allocatable resources for particular frameworks.

We use a new message `ResourceQuantities` to specify resource
requirements. This class is modelled after the internal class of the
same name, and we provide a conversion from the API to the internal
type.


Diffs (updated)
-

  include/mesos/mesos.proto 9d5c2b53164adb44eb4a6f12a507e009bd607940 
  include/mesos/v1/mesos.proto aef31eb5b8781182d3f42d935b12470b319027ed 
  src/common/resource_quantities.hpp 11eb426104577b7977c2307df3e4917085cd 
  src/common/resource_quantities.cpp 320983929cd7d14973c4b98d6ed5338de690ff5f 


Diff: https://reviews.apache.org/r/69818/diff/5/

Changes: https://reviews.apache.org/r/69818/diff/4-5/


Testing
---

`make check`


Thanks,

Benjamin Bannier

Re: Review Request 69819: Added equality operator for `ResourceQuantities`.

[Benjamin Bannier]

> On Jan. 28, 2019, 8:49 p.m., Benjamin Mahler wrote:
> > When we added `ResourceQuantities`, we wanted to support two use cases:
> > 
> > ```
> > guarantees: absent == 0(i.e. zero_by_default)
> > limits: absent == +inf (i.e. inf_by_default)
> > ```
> > 
> > This is why zeros are currently stored (unlike `Resources` objects, where 
> > they are removed): having the ability to distinguish between 0 and absent 
> > allows us to support both use cases. However, the introduction of equality 
> > and containment operations are not so obvious:
> > 
> > ```
> >equality: the approach in this patch is providing inf_by_default 
> > equality semantics AFAICT
> > containment: the approach in the subsequent patch is providing neither 
> > zero_by_default or inf_by_default equality semantics
> > ```
> > 
> > I think that in order to introduce equality and containment, we may have to 
> > address some earlier feedback on `ResourceQuantities`, which was to require 
> > a contruction-time choice between `ZERO_BY_DEFAULT` and `INF_BY_DEFAULT` 
> > semantics. This would be stored as a variable in order to determine (1) 
> > whether to store zeros, (2) how to evaluate equality and containment 
> > between two quantities.
> > 
> > Thoughts?

Yes, that makes sense. I am still unsure whether one should build limit- or 
guarantee-specific functionality into `ResourceQuantities` though as having a 
light-weight building block for more involved semantics, e.g., around quotas.

I am discarding this patch and https://reviews.apache.org/r/69820/ for now as 
they are not needed anymore.


- Benjamin


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


On Jan. 29, 2019, 5:20 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69819/
> ---
> 
> (Updated Jan. 29, 2019, 5:20 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 an `operator==` for `ResourceQuantities`. The operator
> is declared as a `friend` of `ResourceQuantities` so we can directly
> compare the two `ResourceQuantities::quantities` which can be less
> expensive.
> 
> 
> Diffs
> -
> 
>   src/common/resource_quantities.hpp 11eb426104577b7977c2307df3e4917085cd 
>   src/common/resource_quantities.cpp 320983929cd7d14973c4b98d6ed5338de690ff5f 
> 
> 
> Diff: https://reviews.apache.org/r/69819/diff/2/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>