You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Neil Conway <ne...@gmail.com> on 2017/02/28 20:23:24 UTC
Review Request 57167: Updated quota handler logic for hierarchical
roles.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57167/
-----------------------------------------------------------
Review request for mesos and Michael Park.
Repository: mesos
Description
-------
The quota'd resources for a nested role are "included" within the
quota'd resources for that role's parent. Hence, the quota of a node
must always be greater than or equal to the sum of the quota'd resources
of that role's children.
When creating and removing quota, we must ensure that this invariant is
not violated.
When computing the cluster capacity heuristic, we must ensure that we do
not "double-count" quota'd resources: e.g., if the cluster has a total
capacity of 100 cpus, role "x" has a quota guarantee of 60 CPUs, and
role "x/y" has a quota guarantee of 55 CPUs, this does NOT violate the
cluster capacity heuristic.
Diffs
-----
src/master/quota_handler.cpp 3ad28e4a9363a877d0610b529a6c17fb30ece37a
src/tests/hierarchical_allocator_tests.cpp cdf1f15b7802439b28405ca8f6634ce83e886630
src/tests/master_quota_tests.cpp 91219d6693fdd119ed3b0bf734eaa55da9c58b0a
Diff: https://reviews.apache.org/r/57167/diff/
Testing
-------
`make check`
Thanks,
Neil Conway
Re: Review Request 57167: Updated quota handler logic for
hierarchical roles.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57167/#review167200
-----------------------------------------------------------
Bad review!
Reviews applied: [57167, 57166, 56805, 57165, 57164]
Error:
No reviewers specified. Please find a reviewer by asking on JIRA or the mailing list.
- Mesos Reviewbot
On Feb. 28, 2017, 8:23 p.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57167/
> -----------------------------------------------------------
>
> (Updated Feb. 28, 2017, 8:23 p.m.)
>
>
> Review request for mesos and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The quota'd resources for a nested role are "included" within the
> quota'd resources for that role's parent. Hence, the quota of a node
> must always be greater than or equal to the sum of the quota'd resources
> of that role's children.
>
> When creating and removing quota, we must ensure that this invariant is
> not violated.
>
> When computing the cluster capacity heuristic, we must ensure that we do
> not "double-count" quota'd resources: e.g., if the cluster has a total
> capacity of 100 cpus, role "x" has a quota guarantee of 60 CPUs, and
> role "x/y" has a quota guarantee of 55 CPUs, this does NOT violate the
> cluster capacity heuristic.
>
>
> Diffs
> -----
>
> src/master/quota_handler.cpp 3ad28e4a9363a877d0610b529a6c17fb30ece37a
> src/tests/hierarchical_allocator_tests.cpp cdf1f15b7802439b28405ca8f6634ce83e886630
> src/tests/master_quota_tests.cpp 91219d6693fdd119ed3b0bf734eaa55da9c58b0a
>
> Diff: https://reviews.apache.org/r/57167/diff/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 57167: Updated quota handler logic for
hierarchical roles.
Posted by Neil Conway <ne...@gmail.com>.
> On March 9, 2017, 9:39 p.m., Michael Park wrote:
> > src/master/quota_handler.cpp
> > Lines 469-487 (original), 583-605 (patched)
> > <https://reviews.apache.org/r/57167/diff/2/?file=1660312#file1660312line584>
> >
> > I think we should be able to use `strings::tokenize(request.url.path, "/", 3)` here.
Personally I think the current coding is clearer, albeit more verbose. Also `strings::tokenize` skips empty tokens (e.g., `//`), which probably isn't the behavior we want here.
- Neil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57167/#review168525
-----------------------------------------------------------
On March 9, 2017, 4:44 p.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57167/
> -----------------------------------------------------------
>
> (Updated March 9, 2017, 4:44 p.m.)
>
>
> Review request for mesos and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The quota'd resources for a nested role are "included" within the
> quota'd resources for that role's parent. Hence, the quota of a node
> must always be greater than or equal to the sum of the quota'd resources
> of that role's children.
>
> When creating and removing quota, we must ensure that this invariant is
> not violated.
>
> When computing the cluster capacity heuristic, we must ensure that we do
> not "double-count" quota'd resources: e.g., if the cluster has a total
> capacity of 100 CPUs, role "x" has a quota guarantee of 80 CPUs, and
> role "x/y" has a quota guarantee of 40 CPUs, this does NOT violate the
> cluster capacity heuristic.
>
>
> Diffs
> -----
>
> src/master/quota_handler.cpp ce1f0644a56e85a99d8c3742d00940a1bfae3be3
> src/tests/hierarchical_allocator_tests.cpp cdf1f15b7802439b28405ca8f6634ce83e886630
> src/tests/master_quota_tests.cpp e109656492bc5ac65e398b6b61e7321072b162d3
>
>
> Diff: https://reviews.apache.org/r/57167/diff/2/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 57167: Updated quota handler logic for
hierarchical roles.
Posted by Benjamin Mahler <bm...@apache.org>.
> On March 9, 2017, 9:39 p.m., Michael Park wrote:
> > src/master/quota_handler.cpp
> > Lines 469-487 (original), 583-605 (patched)
> > <https://reviews.apache.org/r/57167/diff/2/?file=1660312#file1660312line584>
> >
> > I think we should be able to use `strings::tokenize(request.url.path, "/", 3)` here.
>
> Neil Conway wrote:
> Personally I think the current coding is clearer, albeit more verbose. Also `strings::tokenize` skips empty tokens (e.g., `//`), which probably isn't the behavior we want here.
You can use strings::split if no empty token skipping is desired.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57167/#review168525
-----------------------------------------------------------
On March 10, 2017, 5:04 p.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57167/
> -----------------------------------------------------------
>
> (Updated March 10, 2017, 5:04 p.m.)
>
>
> Review request for mesos and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The quota'd resources for a nested role are "included" within the
> quota'd resources for that role's parent. Hence, the quota of a node
> must always be greater than or equal to the sum of the quota'd resources
> of that role's children.
>
> When creating and removing quota, we must ensure that this invariant is
> not violated.
>
> When computing the cluster capacity heuristic, we must ensure that we do
> not "double-count" quota'd resources: e.g., if the cluster has a total
> capacity of 100 CPUs, role "x" has a quota guarantee of 80 CPUs, and
> role "x/y" has a quota guarantee of 40 CPUs, this does NOT violate the
> cluster capacity heuristic.
>
>
> Diffs
> -----
>
> src/master/quota_handler.cpp 36ea1acca47014b2fb7a3b597b857c8ec9e2ab67
> src/tests/hierarchical_allocator_tests.cpp dce619ec49db480685deb1bf8f7faeebe02e25b5
> src/tests/master_quota_tests.cpp e418f22ea1773f4356ced44b8d57a80e826c8837
>
>
> Diff: https://reviews.apache.org/r/57167/diff/3/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 57167: Updated quota handler logic for
hierarchical roles.
Posted by Michael Park <mp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57167/#review168525
-----------------------------------------------------------
Fix it, then Ship it!
src/master/quota_handler.cpp
Lines 159-168 (patched)
<https://reviews.apache.org/r/57167/#comment240769>
Why not just make this in the constructor?
src/master/quota_handler.cpp
Lines 469-487 (original), 583-605 (patched)
<https://reviews.apache.org/r/57167/#comment240768>
I think we should be able to use `strings::tokenize(request.url.path, "/", 3)` here.
- Michael Park
On March 9, 2017, 8:44 a.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57167/
> -----------------------------------------------------------
>
> (Updated March 9, 2017, 8:44 a.m.)
>
>
> Review request for mesos and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The quota'd resources for a nested role are "included" within the
> quota'd resources for that role's parent. Hence, the quota of a node
> must always be greater than or equal to the sum of the quota'd resources
> of that role's children.
>
> When creating and removing quota, we must ensure that this invariant is
> not violated.
>
> When computing the cluster capacity heuristic, we must ensure that we do
> not "double-count" quota'd resources: e.g., if the cluster has a total
> capacity of 100 CPUs, role "x" has a quota guarantee of 80 CPUs, and
> role "x/y" has a quota guarantee of 40 CPUs, this does NOT violate the
> cluster capacity heuristic.
>
>
> Diffs
> -----
>
> src/master/quota_handler.cpp ce1f0644a56e85a99d8c3742d00940a1bfae3be3
> src/tests/hierarchical_allocator_tests.cpp cdf1f15b7802439b28405ca8f6634ce83e886630
> src/tests/master_quota_tests.cpp e109656492bc5ac65e398b6b61e7321072b162d3
>
>
> Diff: https://reviews.apache.org/r/57167/diff/2/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 57167: Updated quota handler logic for
hierarchical roles.
Posted by Michael Park <mp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57167/#review170761
-----------------------------------------------------------
Ship it!
src/master/quota_handler.cpp
Lines 77-89 (original), 193-205 (patched)
<https://reviews.apache.org/r/57167/#comment243640>
Let's not recompute this stuff here and pass in the total resources into this function instead. We can do this as subsequent work though.
- Michael Park
On March 29, 2017, 7:06 p.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57167/
> -----------------------------------------------------------
>
> (Updated March 29, 2017, 7:06 p.m.)
>
>
> Review request for mesos and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The quota'd resources for a nested role are "included" within the
> quota'd resources for that role's parent. Hence, the quota of a node
> must always be greater than or equal to the sum of the quota'd resources
> of that role's children. When creating and removing quota, we must
> ensure that this invariant is not violated.
>
> When computing the cluster capacity heuristic, we must ensure that we do
> not "double-count" quota'd resources: e.g., if the cluster has a total
> capacity of 100 CPUs, role "x" has a quota guarantee of 80 CPUs, and
> role "x/y" has a quota guarantee of 40 CPUs, this does NOT violate the
> cluster capacity heuristic.
>
>
> Diffs
> -----
>
> src/master/quota_handler.cpp 36ea1acca47014b2fb7a3b597b857c8ec9e2ab67
> src/tests/hierarchical_allocator_tests.cpp e343dc37bd7136f0f6dd5dbc22a25cabe715038d
> src/tests/master_quota_tests.cpp e418f22ea1773f4356ced44b8d57a80e826c8837
>
>
> Diff: https://reviews.apache.org/r/57167/diff/10/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 57167: Updated quota handler logic for
hierarchical roles.
Posted by Neil Conway <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57167/
-----------------------------------------------------------
(Updated March 31, 2017, 10:18 p.m.)
Review request for mesos and Michael Park.
Changes
-------
Tweak whitespace.
Repository: mesos
Description
-------
The quota'd resources for a nested role are "included" within the
quota'd resources for that role's parent. Hence, the quota of a node
must always be greater than or equal to the sum of the quota'd resources
of that role's children. When creating and removing quota, we must
ensure that this invariant is not violated.
When computing the cluster capacity heuristic, we must ensure that we do
not "double-count" quota'd resources: e.g., if the cluster has a total
capacity of 100 CPUs, role "x" has a quota guarantee of 80 CPUs, and
role "x/y" has a quota guarantee of 40 CPUs, this does NOT violate the
cluster capacity heuristic.
Diffs (updated)
-----
src/master/quota_handler.cpp 7ff43a048e17b9e9ac0ceed248f7b3fd56b007d6
src/tests/hierarchical_allocator_tests.cpp e343dc37bd7136f0f6dd5dbc22a25cabe715038d
src/tests/master_quota_tests.cpp 1714ba13ea63bae05448d0898bf722ef472c672b
Diff: https://reviews.apache.org/r/57167/diff/11/
Changes: https://reviews.apache.org/r/57167/diff/10-11/
Testing
-------
`make check`
Thanks,
Neil Conway
Re: Review Request 57167: Updated quota handler logic for
hierarchical roles.
Posted by Neil Conway <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57167/
-----------------------------------------------------------
(Updated March 30, 2017, 2:06 a.m.)
Review request for mesos and Michael Park.
Changes
-------
Simplify request URL parsing logic for quota removal, per discussion.
Repository: mesos
Description (updated)
-------
The quota'd resources for a nested role are "included" within the
quota'd resources for that role's parent. Hence, the quota of a node
must always be greater than or equal to the sum of the quota'd resources
of that role's children. When creating and removing quota, we must
ensure that this invariant is not violated.
When computing the cluster capacity heuristic, we must ensure that we do
not "double-count" quota'd resources: e.g., if the cluster has a total
capacity of 100 CPUs, role "x" has a quota guarantee of 80 CPUs, and
role "x/y" has a quota guarantee of 40 CPUs, this does NOT violate the
cluster capacity heuristic.
Diffs (updated)
-----
src/master/quota_handler.cpp 36ea1acca47014b2fb7a3b597b857c8ec9e2ab67
src/tests/hierarchical_allocator_tests.cpp e343dc37bd7136f0f6dd5dbc22a25cabe715038d
src/tests/master_quota_tests.cpp e418f22ea1773f4356ced44b8d57a80e826c8837
Diff: https://reviews.apache.org/r/57167/diff/10/
Changes: https://reviews.apache.org/r/57167/diff/9-10/
Testing
-------
`make check`
Thanks,
Neil Conway
Re: Review Request 57167: Updated quota handler logic for
hierarchical roles.
Posted by Neil Conway <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57167/
-----------------------------------------------------------
(Updated March 28, 2017, 11:42 p.m.)
Review request for mesos and Michael Park.
Changes
-------
Address review comments.
Repository: mesos
Description
-------
The quota'd resources for a nested role are "included" within the
quota'd resources for that role's parent. Hence, the quota of a node
must always be greater than or equal to the sum of the quota'd resources
of that role's children.
When creating and removing quota, we must ensure that this invariant is
not violated.
When computing the cluster capacity heuristic, we must ensure that we do
not "double-count" quota'd resources: e.g., if the cluster has a total
capacity of 100 CPUs, role "x" has a quota guarantee of 80 CPUs, and
role "x/y" has a quota guarantee of 40 CPUs, this does NOT violate the
cluster capacity heuristic.
Diffs (updated)
-----
src/master/quota_handler.cpp 36ea1acca47014b2fb7a3b597b857c8ec9e2ab67
src/tests/hierarchical_allocator_tests.cpp e343dc37bd7136f0f6dd5dbc22a25cabe715038d
src/tests/master_quota_tests.cpp e418f22ea1773f4356ced44b8d57a80e826c8837
Diff: https://reviews.apache.org/r/57167/diff/9/
Changes: https://reviews.apache.org/r/57167/diff/8-9/
Testing
-------
`make check`
Thanks,
Neil Conway
Re: Review Request 57167: Updated quota handler logic for
hierarchical roles.
Posted by Neil Conway <ne...@gmail.com>.
> On March 28, 2017, 11:06 p.m., Michael Park wrote:
> > src/master/quota_handler.cpp
> > Lines 111-115 (patched)
> > <https://reviews.apache.org/r/57167/diff/8/?file=1672397#file1672397line111>
> >
> > Is this `CHECK` here to require that we only `insert` once per `role`...? If yes, it seems that the comment is a bit confusing to me? Could you clarify the intended semantics here?
Clarified the wording of this comment.
> On March 28, 2017, 11:06 p.m., Michael Park wrote:
> > src/master/quota_handler.cpp
> > Lines 469-487 (original), 605-618 (patched)
> > <https://reviews.apache.org/r/57167/diff/8/?file=1672397#file1672397line605>
> >
> > Couldn't this be just:
> >
> > ```cpp
> > vector<string> components = strings::split(request.url.path, "/", 3u);
> > if (components.size() < 3u) {
> > return BadRequest(
> > "Failed to parse request path '" + request.url.path +
> > "': 3 tokens ('master', 'quota', 'role') required, found " +
> > stringify(tokens.size()) + " token(s)"));
> > }
> >
> > CHECK_EQ(3u, components.size());
> > const string& role = components.back();
> > /* ... */
> > ```
I'm not sure this is actually more readable; leaving as-is for now, will discuss offline.
- Neil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57167/#review170328
-----------------------------------------------------------
On March 23, 2017, 12:53 a.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57167/
> -----------------------------------------------------------
>
> (Updated March 23, 2017, 12:53 a.m.)
>
>
> Review request for mesos and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The quota'd resources for a nested role are "included" within the
> quota'd resources for that role's parent. Hence, the quota of a node
> must always be greater than or equal to the sum of the quota'd resources
> of that role's children.
>
> When creating and removing quota, we must ensure that this invariant is
> not violated.
>
> When computing the cluster capacity heuristic, we must ensure that we do
> not "double-count" quota'd resources: e.g., if the cluster has a total
> capacity of 100 CPUs, role "x" has a quota guarantee of 80 CPUs, and
> role "x/y" has a quota guarantee of 40 CPUs, this does NOT violate the
> cluster capacity heuristic.
>
>
> Diffs
> -----
>
> src/master/quota_handler.cpp 36ea1acca47014b2fb7a3b597b857c8ec9e2ab67
> src/tests/hierarchical_allocator_tests.cpp e343dc37bd7136f0f6dd5dbc22a25cabe715038d
> src/tests/master_quota_tests.cpp e418f22ea1773f4356ced44b8d57a80e826c8837
>
>
> Diff: https://reviews.apache.org/r/57167/diff/8/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 57167: Updated quota handler logic for
hierarchical roles.
Posted by Neil Conway <ne...@gmail.com>.
> On March 28, 2017, 11:06 p.m., Michael Park wrote:
> > src/master/quota_handler.cpp
> > Lines 469-487 (original), 605-618 (patched)
> > <https://reviews.apache.org/r/57167/diff/8/?file=1672397#file1672397line605>
> >
> > Couldn't this be just:
> >
> > ```cpp
> > vector<string> components = strings::split(request.url.path, "/", 3u);
> > if (components.size() < 3u) {
> > return BadRequest(
> > "Failed to parse request path '" + request.url.path +
> > "': 3 tokens ('master', 'quota', 'role') required, found " +
> > stringify(tokens.size()) + " token(s)"));
> > }
> >
> > CHECK_EQ(3u, components.size());
> > const string& role = components.back();
> > /* ... */
> > ```
>
> Neil Conway wrote:
> I'm not sure this is actually more readable; leaving as-is for now, will discuss offline.
>
> Michael Park wrote:
> A summary of our discussion yesterday: the `components.begin() + 3` part looks wrong, but is indeed correct
> because `split` returns empty tokens, and the first token will be an empty string.
> This means that my suggestion of `split(..., 3u)` actually has a bug... subtle :(.
>
> We also didn't like the splitting/joining of the "rest", because it's not obvious that we get back what we put in.
>
> Also, based on the code that I see in libprocess for `route`, it looks like we would allow extraneous slashes such as `/master///quota`.
> This would mean that the use of `split` actually introduces a backwards-incompatibility here.
>
> I personally think it's simplest for us to do `strings::tokenize(request.url.path, "/", 3u)`.
>
> What do you think?
sg!
- Neil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57167/#review170328
-----------------------------------------------------------
On March 28, 2017, 11:42 p.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57167/
> -----------------------------------------------------------
>
> (Updated March 28, 2017, 11:42 p.m.)
>
>
> Review request for mesos and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The quota'd resources for a nested role are "included" within the
> quota'd resources for that role's parent. Hence, the quota of a node
> must always be greater than or equal to the sum of the quota'd resources
> of that role's children.
>
> When creating and removing quota, we must ensure that this invariant is
> not violated.
>
> When computing the cluster capacity heuristic, we must ensure that we do
> not "double-count" quota'd resources: e.g., if the cluster has a total
> capacity of 100 CPUs, role "x" has a quota guarantee of 80 CPUs, and
> role "x/y" has a quota guarantee of 40 CPUs, this does NOT violate the
> cluster capacity heuristic.
>
>
> Diffs
> -----
>
> src/master/quota_handler.cpp 36ea1acca47014b2fb7a3b597b857c8ec9e2ab67
> src/tests/hierarchical_allocator_tests.cpp e343dc37bd7136f0f6dd5dbc22a25cabe715038d
> src/tests/master_quota_tests.cpp e418f22ea1773f4356ced44b8d57a80e826c8837
>
>
> Diff: https://reviews.apache.org/r/57167/diff/9/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 57167: Updated quota handler logic for
hierarchical roles.
Posted by Michael Park <mp...@apache.org>.
> On March 28, 2017, 4:06 p.m., Michael Park wrote:
> > src/master/quota_handler.cpp
> > Lines 469-487 (original), 605-618 (patched)
> > <https://reviews.apache.org/r/57167/diff/8/?file=1672397#file1672397line605>
> >
> > Couldn't this be just:
> >
> > ```cpp
> > vector<string> components = strings::split(request.url.path, "/", 3u);
> > if (components.size() < 3u) {
> > return BadRequest(
> > "Failed to parse request path '" + request.url.path +
> > "': 3 tokens ('master', 'quota', 'role') required, found " +
> > stringify(tokens.size()) + " token(s)"));
> > }
> >
> > CHECK_EQ(3u, components.size());
> > const string& role = components.back();
> > /* ... */
> > ```
>
> Neil Conway wrote:
> I'm not sure this is actually more readable; leaving as-is for now, will discuss offline.
A summary of our discussion yesterday: the `components.begin() + 3` part looks wrong, but is indeed correct
because `split` returns empty tokens, and the first token will be an empty string.
This means that my suggestion of `split(..., 3u)` actually has a bug... subtle :(.
We also didn't like the splitting/joining of the "rest", because it's not obvious that we get back what we put in.
Also, based on the code that I see in libprocess for `route`, it looks like we would allow extraneous slashes such as `/master///quota`.
This would mean that the use of `split` actually introduces a backwards-incompatibility here.
I personally think it's simplest for us to do `strings::tokenize(request.url.path, "/", 3u)`.
What do you think?
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57167/#review170328
-----------------------------------------------------------
On March 28, 2017, 4:42 p.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57167/
> -----------------------------------------------------------
>
> (Updated March 28, 2017, 4:42 p.m.)
>
>
> Review request for mesos and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The quota'd resources for a nested role are "included" within the
> quota'd resources for that role's parent. Hence, the quota of a node
> must always be greater than or equal to the sum of the quota'd resources
> of that role's children.
>
> When creating and removing quota, we must ensure that this invariant is
> not violated.
>
> When computing the cluster capacity heuristic, we must ensure that we do
> not "double-count" quota'd resources: e.g., if the cluster has a total
> capacity of 100 CPUs, role "x" has a quota guarantee of 80 CPUs, and
> role "x/y" has a quota guarantee of 40 CPUs, this does NOT violate the
> cluster capacity heuristic.
>
>
> Diffs
> -----
>
> src/master/quota_handler.cpp 36ea1acca47014b2fb7a3b597b857c8ec9e2ab67
> src/tests/hierarchical_allocator_tests.cpp e343dc37bd7136f0f6dd5dbc22a25cabe715038d
> src/tests/master_quota_tests.cpp e418f22ea1773f4356ced44b8d57a80e826c8837
>
>
> Diff: https://reviews.apache.org/r/57167/diff/9/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 57167: Updated quota handler logic for
hierarchical roles.
Posted by Michael Park <mp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57167/#review170328
-----------------------------------------------------------
Fix it, then Ship it!
src/master/quota_handler.cpp
Lines 111-115 (patched)
<https://reviews.apache.org/r/57167/#comment243132>
Is this `CHECK` here to require that we only `insert` once per `role`...? If yes, it seems that the comment is a bit confusing to me? Could you clarify the intended semantics here?
src/master/quota_handler.cpp
Lines 153-155 (patched)
<https://reviews.apache.org/r/57167/#comment243133>
Let's still put the members at the bottom.
src/master/quota_handler.cpp
Lines 469-487 (original), 605-618 (patched)
<https://reviews.apache.org/r/57167/#comment243127>
Couldn't this be just:
```cpp
vector<string> components = strings::split(request.url.path, "/", 3u);
if (components.size() < 3u) {
return BadRequest(
"Failed to parse request path '" + request.url.path +
"': 3 tokens ('master', 'quota', 'role') required, found " +
stringify(tokens.size()) + " token(s)"));
}
CHECK_EQ(3u, components.size());
const string& role = components.back();
/* ... */
```
src/tests/master_quota_tests.cpp
Lines 1800-1816 (patched)
<https://reviews.apache.org/r/57167/#comment243155>
How about something like this?
```cpp
map<string, Resources> expected = {{parent.role(), parent.guarantee()},
{child.role(), child.guarantee()}};
map<string, Resources> actual = {
{status->infos(0).role(), status->infos(0).guarantee()},
{status->infos(1).role(), status->infos(1).guarantee()}
};
EXPECT_EQ(expected, actual);
```
- Michael Park
On March 22, 2017, 5:53 p.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57167/
> -----------------------------------------------------------
>
> (Updated March 22, 2017, 5:53 p.m.)
>
>
> Review request for mesos and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The quota'd resources for a nested role are "included" within the
> quota'd resources for that role's parent. Hence, the quota of a node
> must always be greater than or equal to the sum of the quota'd resources
> of that role's children.
>
> When creating and removing quota, we must ensure that this invariant is
> not violated.
>
> When computing the cluster capacity heuristic, we must ensure that we do
> not "double-count" quota'd resources: e.g., if the cluster has a total
> capacity of 100 CPUs, role "x" has a quota guarantee of 80 CPUs, and
> role "x/y" has a quota guarantee of 40 CPUs, this does NOT violate the
> cluster capacity heuristic.
>
>
> Diffs
> -----
>
> src/master/quota_handler.cpp 36ea1acca47014b2fb7a3b597b857c8ec9e2ab67
> src/tests/hierarchical_allocator_tests.cpp e343dc37bd7136f0f6dd5dbc22a25cabe715038d
> src/tests/master_quota_tests.cpp e418f22ea1773f4356ced44b8d57a80e826c8837
>
>
> Diff: https://reviews.apache.org/r/57167/diff/8/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 57167: Updated quota handler logic for
hierarchical roles.
Posted by Neil Conway <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57167/
-----------------------------------------------------------
(Updated March 23, 2017, 12:53 a.m.)
Review request for mesos and Michael Park.
Changes
-------
Add extra test.
Repository: mesos
Description
-------
The quota'd resources for a nested role are "included" within the
quota'd resources for that role's parent. Hence, the quota of a node
must always be greater than or equal to the sum of the quota'd resources
of that role's children.
When creating and removing quota, we must ensure that this invariant is
not violated.
When computing the cluster capacity heuristic, we must ensure that we do
not "double-count" quota'd resources: e.g., if the cluster has a total
capacity of 100 CPUs, role "x" has a quota guarantee of 80 CPUs, and
role "x/y" has a quota guarantee of 40 CPUs, this does NOT violate the
cluster capacity heuristic.
Diffs (updated)
-----
src/master/quota_handler.cpp 36ea1acca47014b2fb7a3b597b857c8ec9e2ab67
src/tests/hierarchical_allocator_tests.cpp e343dc37bd7136f0f6dd5dbc22a25cabe715038d
src/tests/master_quota_tests.cpp e418f22ea1773f4356ced44b8d57a80e826c8837
Diff: https://reviews.apache.org/r/57167/diff/8/
Changes: https://reviews.apache.org/r/57167/diff/7-8/
Testing
-------
`make check`
Thanks,
Neil Conway
Re: Review Request 57167: Updated quota handler logic for
hierarchical roles.
Posted by Neil Conway <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57167/
-----------------------------------------------------------
(Updated March 22, 2017, 5:56 p.m.)
Review request for mesos and Michael Park.
Changes
-------
Tweak whitespace per review comment.
Repository: mesos
Description
-------
The quota'd resources for a nested role are "included" within the
quota'd resources for that role's parent. Hence, the quota of a node
must always be greater than or equal to the sum of the quota'd resources
of that role's children.
When creating and removing quota, we must ensure that this invariant is
not violated.
When computing the cluster capacity heuristic, we must ensure that we do
not "double-count" quota'd resources: e.g., if the cluster has a total
capacity of 100 CPUs, role "x" has a quota guarantee of 80 CPUs, and
role "x/y" has a quota guarantee of 40 CPUs, this does NOT violate the
cluster capacity heuristic.
Diffs (updated)
-----
src/master/quota_handler.cpp 36ea1acca47014b2fb7a3b597b857c8ec9e2ab67
src/tests/hierarchical_allocator_tests.cpp e343dc37bd7136f0f6dd5dbc22a25cabe715038d
src/tests/master_quota_tests.cpp e418f22ea1773f4356ced44b8d57a80e826c8837
Diff: https://reviews.apache.org/r/57167/diff/7/
Changes: https://reviews.apache.org/r/57167/diff/6-7/
Testing
-------
`make check`
Thanks,
Neil Conway
Re: Review Request 57167: Updated quota handler logic for
hierarchical roles.
Posted by Neil Conway <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57167/
-----------------------------------------------------------
(Updated March 17, 2017, 2:10 a.m.)
Review request for mesos and Michael Park.
Changes
-------
Better error reporting per mpark's suggestion.
Repository: mesos
Description
-------
The quota'd resources for a nested role are "included" within the
quota'd resources for that role's parent. Hence, the quota of a node
must always be greater than or equal to the sum of the quota'd resources
of that role's children.
When creating and removing quota, we must ensure that this invariant is
not violated.
When computing the cluster capacity heuristic, we must ensure that we do
not "double-count" quota'd resources: e.g., if the cluster has a total
capacity of 100 CPUs, role "x" has a quota guarantee of 80 CPUs, and
role "x/y" has a quota guarantee of 40 CPUs, this does NOT violate the
cluster capacity heuristic.
Diffs (updated)
-----
src/master/quota_handler.cpp 36ea1acca47014b2fb7a3b597b857c8ec9e2ab67
src/tests/hierarchical_allocator_tests.cpp e343dc37bd7136f0f6dd5dbc22a25cabe715038d
src/tests/master_quota_tests.cpp e418f22ea1773f4356ced44b8d57a80e826c8837
Diff: https://reviews.apache.org/r/57167/diff/6/
Changes: https://reviews.apache.org/r/57167/diff/5-6/
Testing
-------
`make check`
Thanks,
Neil Conway
Re: Review Request 57167: Updated quota handler logic for
hierarchical roles.
Posted by Neil Conway <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57167/
-----------------------------------------------------------
(Updated March 16, 2017, 4:53 p.m.)
Review request for mesos and Michael Park.
Changes
-------
Fix typo in comment.
Repository: mesos
Description
-------
The quota'd resources for a nested role are "included" within the
quota'd resources for that role's parent. Hence, the quota of a node
must always be greater than or equal to the sum of the quota'd resources
of that role's children.
When creating and removing quota, we must ensure that this invariant is
not violated.
When computing the cluster capacity heuristic, we must ensure that we do
not "double-count" quota'd resources: e.g., if the cluster has a total
capacity of 100 CPUs, role "x" has a quota guarantee of 80 CPUs, and
role "x/y" has a quota guarantee of 40 CPUs, this does NOT violate the
cluster capacity heuristic.
Diffs (updated)
-----
src/master/quota_handler.cpp 36ea1acca47014b2fb7a3b597b857c8ec9e2ab67
src/tests/hierarchical_allocator_tests.cpp dce619ec49db480685deb1bf8f7faeebe02e25b5
src/tests/master_quota_tests.cpp e418f22ea1773f4356ced44b8d57a80e826c8837
Diff: https://reviews.apache.org/r/57167/diff/5/
Changes: https://reviews.apache.org/r/57167/diff/4-5/
Testing
-------
`make check`
Thanks,
Neil Conway
Re: Review Request 57167: Updated quota handler logic for
hierarchical roles.
Posted by Jay Guo <gu...@gmail.com>.
> On March 16, 2017, 2:54 p.m., Jay Guo wrote:
> > What should be the correct behavior for following case:
> > - parent role `/a` is quota'd with `cpus:2;mem:1024`
> > - child role `/a/b` is quota'd with `cpus:1;mem:512`
> > - `Framework1` subscribes to role `/a`
> > - `Framework2` subscribes to role `/a/b`
> >
> > From my understanding:
> > - `cpus:1;mem:512` should be offered to `Framework2`
> > - remaining `cpus:1;mem:512` should be fairly shared by two frameworks, resulting in `cpus:0.5;mem:256` for `Framework1` and `cpus:1.5;mem:768` for `Framework2`, since virtual role (which `Framework1` subscribes to) always has default quota of zero.
> >
> > Am I missing something?
> >
> > Another thought is that we should document the correct way to set quota for hierarchical roles (always set quota for parent and then children).
My question is that how to quota an internal node, if virtual node always has zero quota.
- Jay
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57167/#review169098
-----------------------------------------------------------
On March 11, 2017, 6:03 a.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57167/
> -----------------------------------------------------------
>
> (Updated March 11, 2017, 6:03 a.m.)
>
>
> Review request for mesos and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The quota'd resources for a nested role are "included" within the
> quota'd resources for that role's parent. Hence, the quota of a node
> must always be greater than or equal to the sum of the quota'd resources
> of that role's children.
>
> When creating and removing quota, we must ensure that this invariant is
> not violated.
>
> When computing the cluster capacity heuristic, we must ensure that we do
> not "double-count" quota'd resources: e.g., if the cluster has a total
> capacity of 100 CPUs, role "x" has a quota guarantee of 80 CPUs, and
> role "x/y" has a quota guarantee of 40 CPUs, this does NOT violate the
> cluster capacity heuristic.
>
>
> Diffs
> -----
>
> src/master/quota_handler.cpp 36ea1acca47014b2fb7a3b597b857c8ec9e2ab67
> src/tests/hierarchical_allocator_tests.cpp dce619ec49db480685deb1bf8f7faeebe02e25b5
> src/tests/master_quota_tests.cpp e418f22ea1773f4356ced44b8d57a80e826c8837
>
>
> Diff: https://reviews.apache.org/r/57167/diff/4/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 57167: Updated quota handler logic for
hierarchical roles.
Posted by Neil Conway <ne...@gmail.com>.
> On March 16, 2017, 6:54 a.m., Jay Guo wrote:
> > What should be the correct behavior for following case:
> > - parent role `/a` is quota'd with `cpus:2;mem:1024`
> > - child role `/a/b` is quota'd with `cpus:1;mem:512`
> > - `Framework1` subscribes to role `/a`
> > - `Framework2` subscribes to role `/a/b`
> >
> > From my understanding:
> > - `cpus:1;mem:512` should be offered to `Framework2`
> > - remaining `cpus:1;mem:512` should be fairly shared by two frameworks, resulting in `cpus:0.5;mem:256` for `Framework1` and `cpus:1.5;mem:768` for `Framework2`, since virtual role (which `Framework1` subscribes to) always has default quota of zero.
> >
> > Am I missing something?
> >
> > Another thought is that we should document the correct way to set quota for hierarchical roles (always set quota for parent and then children).
>
> Jay Guo wrote:
> My question is that how to quota an internal node, if virtual node always has zero quota.
Hi Jay.
We're referring to that scenario as "quota delegation". In the current RR chain, the additional `cpus:1;mem:512` will _only_ be offered to `Framework1`. We (probably) want to change this behavior so that these resources are fair-shared among all the roles in the `a` subtree, but that behavior is not implemented yet. This behavior isn't specific to virtual leaf nodes as such. I added an extra test (`HierarchicalAllocatorTest.NestedRoleQuotaAllocateToParent`) to https://reviews.apache.org/r/57254/ to cover this case more explicitly.
Let me know if that makes sense!
Neil
- Neil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57167/#review169098
-----------------------------------------------------------
On March 16, 2017, 4:53 p.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57167/
> -----------------------------------------------------------
>
> (Updated March 16, 2017, 4:53 p.m.)
>
>
> Review request for mesos and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The quota'd resources for a nested role are "included" within the
> quota'd resources for that role's parent. Hence, the quota of a node
> must always be greater than or equal to the sum of the quota'd resources
> of that role's children.
>
> When creating and removing quota, we must ensure that this invariant is
> not violated.
>
> When computing the cluster capacity heuristic, we must ensure that we do
> not "double-count" quota'd resources: e.g., if the cluster has a total
> capacity of 100 CPUs, role "x" has a quota guarantee of 80 CPUs, and
> role "x/y" has a quota guarantee of 40 CPUs, this does NOT violate the
> cluster capacity heuristic.
>
>
> Diffs
> -----
>
> src/master/quota_handler.cpp 36ea1acca47014b2fb7a3b597b857c8ec9e2ab67
> src/tests/hierarchical_allocator_tests.cpp dce619ec49db480685deb1bf8f7faeebe02e25b5
> src/tests/master_quota_tests.cpp e418f22ea1773f4356ced44b8d57a80e826c8837
>
>
> Diff: https://reviews.apache.org/r/57167/diff/5/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 57167: Updated quota handler logic for
hierarchical roles.
Posted by Jay Guo <gu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57167/#review169098
-----------------------------------------------------------
What should be the correct behavior for following case:
- parent role `/a` is quota'd with `cpus:2;mem:1024`
- child role `/a/b` is quota'd with `cpus:1;mem:512`
- `Framework1` subscribes to role `/a`
- `Framework2` subscribes to role `/a/b`
From my understanding:
- `cpus:1;mem:512` should be offered to `Framework2`
- remaining `cpus:1;mem:512` should be fairly shared by two frameworks, resulting in `cpus:0.5;mem:256` for `Framework1` and `cpus:1.5;mem:768` for `Framework2`, since virtual role (which `Framework1` subscribes to) always has default quota of zero.
Am I missing something?
Another thought is that we should document the correct way to set quota for hierarchical roles (always set quota for parent and then children).
src/tests/master_quota_tests.cpp
Lines 1918 (patched)
<https://reviews.apache.org/r/57167/#comment241421>
s/first/second/
- Jay Guo
On March 11, 2017, 6:03 a.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57167/
> -----------------------------------------------------------
>
> (Updated March 11, 2017, 6:03 a.m.)
>
>
> Review request for mesos and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The quota'd resources for a nested role are "included" within the
> quota'd resources for that role's parent. Hence, the quota of a node
> must always be greater than or equal to the sum of the quota'd resources
> of that role's children.
>
> When creating and removing quota, we must ensure that this invariant is
> not violated.
>
> When computing the cluster capacity heuristic, we must ensure that we do
> not "double-count" quota'd resources: e.g., if the cluster has a total
> capacity of 100 CPUs, role "x" has a quota guarantee of 80 CPUs, and
> role "x/y" has a quota guarantee of 40 CPUs, this does NOT violate the
> cluster capacity heuristic.
>
>
> Diffs
> -----
>
> src/master/quota_handler.cpp 36ea1acca47014b2fb7a3b597b857c8ec9e2ab67
> src/tests/hierarchical_allocator_tests.cpp dce619ec49db480685deb1bf8f7faeebe02e25b5
> src/tests/master_quota_tests.cpp e418f22ea1773f4356ced44b8d57a80e826c8837
>
>
> Diff: https://reviews.apache.org/r/57167/diff/4/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 57167: Updated quota handler logic for
hierarchical roles.
Posted by Neil Conway <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57167/
-----------------------------------------------------------
(Updated March 10, 2017, 10:03 p.m.)
Review request for mesos and Michael Park.
Changes
-------
Address review comments.
Repository: mesos
Description
-------
The quota'd resources for a nested role are "included" within the
quota'd resources for that role's parent. Hence, the quota of a node
must always be greater than or equal to the sum of the quota'd resources
of that role's children.
When creating and removing quota, we must ensure that this invariant is
not violated.
When computing the cluster capacity heuristic, we must ensure that we do
not "double-count" quota'd resources: e.g., if the cluster has a total
capacity of 100 CPUs, role "x" has a quota guarantee of 80 CPUs, and
role "x/y" has a quota guarantee of 40 CPUs, this does NOT violate the
cluster capacity heuristic.
Diffs (updated)
-----
src/master/quota_handler.cpp 36ea1acca47014b2fb7a3b597b857c8ec9e2ab67
src/tests/hierarchical_allocator_tests.cpp dce619ec49db480685deb1bf8f7faeebe02e25b5
src/tests/master_quota_tests.cpp e418f22ea1773f4356ced44b8d57a80e826c8837
Diff: https://reviews.apache.org/r/57167/diff/4/
Changes: https://reviews.apache.org/r/57167/diff/3-4/
Testing
-------
`make check`
Thanks,
Neil Conway
Re: Review Request 57167: Updated quota handler logic for
hierarchical roles.
Posted by Michael Park <mp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57167/#review168670
-----------------------------------------------------------
src/master/quota_handler.cpp
Lines 141 (patched)
<https://reviews.apache.org/r/57167/#comment240947>
There are a few different terminologies being introduced I think. `path`, `name`, `label`. I think `name` == `label` currently? We should settle with one. I imagine `name` would be better than `label`?
src/master/quota_handler.cpp
Lines 87-89 (original), 184-186 (patched)
<https://reviews.apache.org/r/57167/#comment240950>
Should we be calling `validate` here?
- Michael Park
On March 10, 2017, 9:04 a.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57167/
> -----------------------------------------------------------
>
> (Updated March 10, 2017, 9:04 a.m.)
>
>
> Review request for mesos and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The quota'd resources for a nested role are "included" within the
> quota'd resources for that role's parent. Hence, the quota of a node
> must always be greater than or equal to the sum of the quota'd resources
> of that role's children.
>
> When creating and removing quota, we must ensure that this invariant is
> not violated.
>
> When computing the cluster capacity heuristic, we must ensure that we do
> not "double-count" quota'd resources: e.g., if the cluster has a total
> capacity of 100 CPUs, role "x" has a quota guarantee of 80 CPUs, and
> role "x/y" has a quota guarantee of 40 CPUs, this does NOT violate the
> cluster capacity heuristic.
>
>
> Diffs
> -----
>
> src/master/quota_handler.cpp 36ea1acca47014b2fb7a3b597b857c8ec9e2ab67
> src/tests/hierarchical_allocator_tests.cpp dce619ec49db480685deb1bf8f7faeebe02e25b5
> src/tests/master_quota_tests.cpp e418f22ea1773f4356ced44b8d57a80e826c8837
>
>
> Diff: https://reviews.apache.org/r/57167/diff/3/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 57167: Updated quota handler logic for
hierarchical roles.
Posted by Neil Conway <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57167/
-----------------------------------------------------------
(Updated March 10, 2017, 5:04 p.m.)
Review request for mesos and Michael Park.
Changes
-------
Address review comment, whitespace fixes.
Repository: mesos
Description
-------
The quota'd resources for a nested role are "included" within the
quota'd resources for that role's parent. Hence, the quota of a node
must always be greater than or equal to the sum of the quota'd resources
of that role's children.
When creating and removing quota, we must ensure that this invariant is
not violated.
When computing the cluster capacity heuristic, we must ensure that we do
not "double-count" quota'd resources: e.g., if the cluster has a total
capacity of 100 CPUs, role "x" has a quota guarantee of 80 CPUs, and
role "x/y" has a quota guarantee of 40 CPUs, this does NOT violate the
cluster capacity heuristic.
Diffs (updated)
-----
src/master/quota_handler.cpp 36ea1acca47014b2fb7a3b597b857c8ec9e2ab67
src/tests/hierarchical_allocator_tests.cpp dce619ec49db480685deb1bf8f7faeebe02e25b5
src/tests/master_quota_tests.cpp e418f22ea1773f4356ced44b8d57a80e826c8837
Diff: https://reviews.apache.org/r/57167/diff/3/
Changes: https://reviews.apache.org/r/57167/diff/2-3/
Testing
-------
`make check`
Thanks,
Neil Conway
Re: Review Request 57167: Updated quota handler logic for
hierarchical roles.
Posted by Neil Conway <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57167/
-----------------------------------------------------------
(Updated March 9, 2017, 4:44 p.m.)
Review request for mesos and Michael Park.
Changes
-------
Introduce `QuotaTree`, other fixes.
Repository: mesos
Description (updated)
-------
The quota'd resources for a nested role are "included" within the
quota'd resources for that role's parent. Hence, the quota of a node
must always be greater than or equal to the sum of the quota'd resources
of that role's children.
When creating and removing quota, we must ensure that this invariant is
not violated.
When computing the cluster capacity heuristic, we must ensure that we do
not "double-count" quota'd resources: e.g., if the cluster has a total
capacity of 100 CPUs, role "x" has a quota guarantee of 80 CPUs, and
role "x/y" has a quota guarantee of 40 CPUs, this does NOT violate the
cluster capacity heuristic.
Diffs (updated)
-----
src/master/quota_handler.cpp ce1f0644a56e85a99d8c3742d00940a1bfae3be3
src/tests/hierarchical_allocator_tests.cpp cdf1f15b7802439b28405ca8f6634ce83e886630
src/tests/master_quota_tests.cpp e109656492bc5ac65e398b6b61e7321072b162d3
Diff: https://reviews.apache.org/r/57167/diff/2/
Changes: https://reviews.apache.org/r/57167/diff/1-2/
Testing
-------
`make check`
Thanks,
Neil Conway
Re: Review Request 57167: Updated quota handler logic for
hierarchical roles.
Posted by Neil Conway <ne...@gmail.com>.
> On March 8, 2017, 6:38 p.m., Michael Park wrote:
> > src/master/quota_handler.cpp
> > Lines 70 (patched)
> > <https://reviews.apache.org/r/57167/diff/1/?file=1652005#file1652005line70>
> >
> > What do you think of introducing a `QuotaTree` with which we can encapsulate all this stuff?
> >
> > Something like:
> >
> > ```cpp
> > class QuotaTree {
> > QuotaTree() = default;
> >
> > Try<Nothing> insert(const string& role, const Quota& quota) const {
> > // basically the body of `foreachpair` in `buildQuotaTree` + validation.
> > }
> >
> > Resources total() const;
> >
> > private:
> >
> > class Node { ... };
> >
> > unique_ptr<Node> root;
> > };
> >
> > Try<QuotaTree> buildQuotaTree(const hashmap<string, Quota>& quotas)
> > {
> > QuotaTree result;
> >
> > foreachpair (cons string& role, const Quota& quota, quotas) {
> > Try<Nothing> insert = result.insert(role, quota);
> > if (insert.isError()) {
> > return Error("Failed to build quota tree" + insert.error());
> > }
> > }
> >
> > return result;
> > }
> > ```
> >
> > This way we could also keep a running `quotaTree` rather than a `quotaMap` and rebuilding the `quotaTree` when we need it. Perhaps a minor point. Even if we want to simply keep what we have in this patch, I think having a:
> >
> > ```cpp
> > class QuotaTree {
> > QuotaTree(const hashmap<string, Quota>&);
> > Option<Error> validate() const;
> > Resources total() const;
> >
> > // private stuff...
> > };
> > ```
> >
> > could simplify the usage a little bit.
This is a nice cleanup -- thanks for the suggestion.
- Neil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57167/#review168318
-----------------------------------------------------------
On March 9, 2017, 4:44 p.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57167/
> -----------------------------------------------------------
>
> (Updated March 9, 2017, 4:44 p.m.)
>
>
> Review request for mesos and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The quota'd resources for a nested role are "included" within the
> quota'd resources for that role's parent. Hence, the quota of a node
> must always be greater than or equal to the sum of the quota'd resources
> of that role's children.
>
> When creating and removing quota, we must ensure that this invariant is
> not violated.
>
> When computing the cluster capacity heuristic, we must ensure that we do
> not "double-count" quota'd resources: e.g., if the cluster has a total
> capacity of 100 CPUs, role "x" has a quota guarantee of 80 CPUs, and
> role "x/y" has a quota guarantee of 40 CPUs, this does NOT violate the
> cluster capacity heuristic.
>
>
> Diffs
> -----
>
> src/master/quota_handler.cpp ce1f0644a56e85a99d8c3742d00940a1bfae3be3
> src/tests/hierarchical_allocator_tests.cpp cdf1f15b7802439b28405ca8f6634ce83e886630
> src/tests/master_quota_tests.cpp e109656492bc5ac65e398b6b61e7321072b162d3
>
>
> Diff: https://reviews.apache.org/r/57167/diff/2/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 57167: Updated quota handler logic for
hierarchical roles.
Posted by Neil Conway <ne...@gmail.com>.
> On March 8, 2017, 6:38 p.m., Michael Park wrote:
> > src/master/quota_handler.cpp
> > Lines 78 (patched)
> > <https://reviews.apache.org/r/57167/diff/1/?file=1652005#file1652005line78>
> >
> > The general pattern for `validate` I think is to return an `Option<Error>`. In this case, we could more accurately report what/where the invalid relationship is within the tree.
`Option<Error>` seemed like overkill to me, and doesn't compose as nicely when checking the validity of children. I renamed the member function to `isValid()` -- how does that sound?
- Neil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57167/#review168318
-----------------------------------------------------------
On March 10, 2017, 5:04 p.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57167/
> -----------------------------------------------------------
>
> (Updated March 10, 2017, 5:04 p.m.)
>
>
> Review request for mesos and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The quota'd resources for a nested role are "included" within the
> quota'd resources for that role's parent. Hence, the quota of a node
> must always be greater than or equal to the sum of the quota'd resources
> of that role's children.
>
> When creating and removing quota, we must ensure that this invariant is
> not violated.
>
> When computing the cluster capacity heuristic, we must ensure that we do
> not "double-count" quota'd resources: e.g., if the cluster has a total
> capacity of 100 CPUs, role "x" has a quota guarantee of 80 CPUs, and
> role "x/y" has a quota guarantee of 40 CPUs, this does NOT violate the
> cluster capacity heuristic.
>
>
> Diffs
> -----
>
> src/master/quota_handler.cpp 36ea1acca47014b2fb7a3b597b857c8ec9e2ab67
> src/tests/hierarchical_allocator_tests.cpp dce619ec49db480685deb1bf8f7faeebe02e25b5
> src/tests/master_quota_tests.cpp e418f22ea1773f4356ced44b8d57a80e826c8837
>
>
> Diff: https://reviews.apache.org/r/57167/diff/3/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 57167: Updated quota handler logic for
hierarchical roles.
Posted by Michael Park <mp...@apache.org>.
> On March 8, 2017, 10:38 a.m., Michael Park wrote:
> > src/master/quota_handler.cpp
> > Lines 70 (patched)
> > <https://reviews.apache.org/r/57167/diff/1/?file=1652005#file1652005line70>
> >
> > What do you think of introducing a `QuotaTree` with which we can encapsulate all this stuff?
> >
> > Something like:
> >
> > ```cpp
> > class QuotaTree {
> > QuotaTree() = default;
> >
> > Try<Nothing> insert(const string& role, const Quota& quota) const {
> > // basically the body of `foreachpair` in `buildQuotaTree` + validation.
> > }
> >
> > Resources total() const;
> >
> > private:
> >
> > class Node { ... };
> >
> > unique_ptr<Node> root;
> > };
> >
> > Try<QuotaTree> buildQuotaTree(const hashmap<string, Quota>& quotas)
> > {
> > QuotaTree result;
> >
> > foreachpair (cons string& role, const Quota& quota, quotas) {
> > Try<Nothing> insert = result.insert(role, quota);
> > if (insert.isError()) {
> > return Error("Failed to build quota tree" + insert.error());
> > }
> > }
> >
> > return result;
> > }
> > ```
> >
> > This way we could also keep a running `quotaTree` rather than a `quotaMap` and rebuilding the `quotaTree` when we need it. Perhaps a minor point. Even if we want to simply keep what we have in this patch, I think having a:
> >
> > ```cpp
> > class QuotaTree {
> > QuotaTree(const hashmap<string, Quota>&);
> > Option<Error> validate() const;
> > Resources total() const;
> >
> > // private stuff...
> > };
> > ```
> >
> > could simplify the usage a little bit.
>
> Neil Conway wrote:
> This is a nice cleanup -- thanks for the suggestion.
Leaving a breadcrumb here as to why we currently don't have a running `quotaTree`:
> Re: keeping around `QuotaTree` rather than rebuilding it, that would require implementing `remove`,
>
> and in general might introduce bugs about not properly updating the state of the tree over time.
>
> Since the perf cost of rebuilding the tree is insignificant, i'd opt for continuing to rebuild it.
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57167/#review168318
-----------------------------------------------------------
On March 31, 2017, 3:18 p.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57167/
> -----------------------------------------------------------
>
> (Updated March 31, 2017, 3:18 p.m.)
>
>
> Review request for mesos and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The quota'd resources for a nested role are "included" within the
> quota'd resources for that role's parent. Hence, the quota of a node
> must always be greater than or equal to the sum of the quota'd resources
> of that role's children. When creating and removing quota, we must
> ensure that this invariant is not violated.
>
> When computing the cluster capacity heuristic, we must ensure that we do
> not "double-count" quota'd resources: e.g., if the cluster has a total
> capacity of 100 CPUs, role "x" has a quota guarantee of 80 CPUs, and
> role "x/y" has a quota guarantee of 40 CPUs, this does NOT violate the
> cluster capacity heuristic.
>
>
> Diffs
> -----
>
> src/master/quota_handler.cpp 7ff43a048e17b9e9ac0ceed248f7b3fd56b007d6
> src/tests/hierarchical_allocator_tests.cpp e343dc37bd7136f0f6dd5dbc22a25cabe715038d
> src/tests/master_quota_tests.cpp 1714ba13ea63bae05448d0898bf722ef472c672b
>
>
> Diff: https://reviews.apache.org/r/57167/diff/11/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 57167: Updated quota handler logic for
hierarchical roles.
Posted by Michael Park <mp...@apache.org>.
> On March 8, 2017, 10:38 a.m., Michael Park wrote:
> > src/master/quota_handler.cpp
> > Lines 78 (patched)
> > <https://reviews.apache.org/r/57167/diff/1/?file=1652005#file1652005line78>
> >
> > The general pattern for `validate` I think is to return an `Option<Error>`. In this case, we could more accurately report what/where the invalid relationship is within the tree.
>
> Neil Conway wrote:
> `Option<Error>` seemed like overkill to me, and doesn't compose as nicely when checking the validity of children. I renamed the member function to `isValid()` -- how does that sound?
> Option<Error> seemed like overkill to me
Okay
> and doesn't compose as nicely when checking the validity of children.
Hm...? I don't get it. Doesn't it just go from
```cpp
bool isValid() const
{
foreachvalue (const unique_ptr<Node>& child, root->children) {
if (!child->isValid()) {
return false;
}
}
return true;
}
```
to...
```cpp
Option<Error> validate() const
{
foreachvalue (const unique_ptr<Node>& child, root->children) {
Option<Error> validate = child->validate();
if (validate.isSome()) {
return validate;
}
}
return None();
}
```
and,
```cpp
bool isValid() const
{
foreachvalue (const unique_ptr<Node>& child, children) {
if (!child->isValid()) {
return false;
}
}
Resources childResources;
foreachvalue (const unique_ptr<Node>& child, children) {
childResources += child->quota.info.guarantee();
}
Resources selfResources = quota.info.guarantee();
return selfResources.contains(childResources);
}
```
to...
```cpp
Option<Error> validate() const
{
foreachvalue (const unique_ptr<Node>& child, children) {
Option<Error> validate = child->validate();
if (validate.isSome()) {
return validate;
}
}
Resources childResources;
foreachvalue (const unique_ptr<Node>& child, children) {
childResources += child->quota.info.guarantee();
}
Resources selfResources = quota.info.guarantee();
if (!selfResources.contains(childResources)) {
return Error("Detected invalid quota tree. Parent role " + name +
" with quota: " + selfResources + " does not contain"
"the sum of its children's resources: " + childResources);
}
return None();
}
```
It's fine if you think `validate` is overkill, but I'm not seeing what doesn't work out nicely.
To me it just seems like it's not any more complicated, and produces a better error message for the user.
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57167/#review168318
-----------------------------------------------------------
On March 10, 2017, 2:03 p.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57167/
> -----------------------------------------------------------
>
> (Updated March 10, 2017, 2:03 p.m.)
>
>
> Review request for mesos and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The quota'd resources for a nested role are "included" within the
> quota'd resources for that role's parent. Hence, the quota of a node
> must always be greater than or equal to the sum of the quota'd resources
> of that role's children.
>
> When creating and removing quota, we must ensure that this invariant is
> not violated.
>
> When computing the cluster capacity heuristic, we must ensure that we do
> not "double-count" quota'd resources: e.g., if the cluster has a total
> capacity of 100 CPUs, role "x" has a quota guarantee of 80 CPUs, and
> role "x/y" has a quota guarantee of 40 CPUs, this does NOT violate the
> cluster capacity heuristic.
>
>
> Diffs
> -----
>
> src/master/quota_handler.cpp 36ea1acca47014b2fb7a3b597b857c8ec9e2ab67
> src/tests/hierarchical_allocator_tests.cpp dce619ec49db480685deb1bf8f7faeebe02e25b5
> src/tests/master_quota_tests.cpp e418f22ea1773f4356ced44b8d57a80e826c8837
>
>
> Diff: https://reviews.apache.org/r/57167/diff/4/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 57167: Updated quota handler logic for
hierarchical roles.
Posted by Michael Park <mp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57167/#review168318
-----------------------------------------------------------
src/master/quota_handler.cpp
Lines 70 (patched)
<https://reviews.apache.org/r/57167/#comment240536>
What do you think of introducing a `QuotaTree` with which we can encapsulate all this stuff?
Something like:
```cpp
class QuotaTree {
QuotaTree() = default;
Try<Nothing> insert(const string& role, const Quota& quota) const {
// basically the body of `foreachpair` in `buildQuotaTree` + validation.
}
Resources total() const;
private:
class Node { ... };
unique_ptr<Node> root;
};
Try<QuotaTree> buildQuotaTree(const hashmap<string, Quota>& quotas)
{
QuotaTree result;
foreachpair (cons string& role, const Quota& quota, quotas) {
Try<Nothing> insert = result.insert(role, quota);
if (insert.isError()) {
return Error("Failed to build quota tree" + insert.error());
}
}
return result;
}
```
This way we could also keep a running `quotaTree` rather than a `quotaMap` and rebuilding the `quotaTree` when we need it. Perhaps a minor point. Even if we want to simply keep what we have in this patch, I think having a:
```cpp
class QuotaTree {
QuotaTree(const hashmap<string, Quota>&);
Option<Error> validate() const;
Resources total() const;
// private stuff...
};
```
could simplify the usage a little bit.
src/master/quota_handler.cpp
Lines 78 (patched)
<https://reviews.apache.org/r/57167/#comment240532>
The general pattern for `validate` I think is to return an `Option<Error>`. In this case, we could more accurately report what/where the invalid relationship is within the tree.
src/master/quota_handler.cpp
Lines 87-89 (patched)
<https://reviews.apache.org/r/57167/#comment240533>
We can just use the `Resources` implicit constructor here.
```cpp
childResources += child->quota.info.guarantee()
```
src/master/quota_handler.cpp
Lines 92-95 (patched)
<https://reviews.apache.org/r/57167/#comment240534>
Ditto.
```cpp
selfResources = quota.info.guarantee()
```
src/master/quota_handler.cpp
Lines 102 (patched)
<https://reviews.apache.org/r/57167/#comment240535>
Is `quotas` meant to be taken by value here?
- Michael Park
On Feb. 28, 2017, 12:23 p.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57167/
> -----------------------------------------------------------
>
> (Updated Feb. 28, 2017, 12:23 p.m.)
>
>
> Review request for mesos and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The quota'd resources for a nested role are "included" within the
> quota'd resources for that role's parent. Hence, the quota of a node
> must always be greater than or equal to the sum of the quota'd resources
> of that role's children.
>
> When creating and removing quota, we must ensure that this invariant is
> not violated.
>
> When computing the cluster capacity heuristic, we must ensure that we do
> not "double-count" quota'd resources: e.g., if the cluster has a total
> capacity of 100 cpus, role "x" has a quota guarantee of 60 CPUs, and
> role "x/y" has a quota guarantee of 55 CPUs, this does NOT violate the
> cluster capacity heuristic.
>
>
> Diffs
> -----
>
> src/master/quota_handler.cpp 3ad28e4a9363a877d0610b529a6c17fb30ece37a
> src/tests/hierarchical_allocator_tests.cpp cdf1f15b7802439b28405ca8f6634ce83e886630
> src/tests/master_quota_tests.cpp 91219d6693fdd119ed3b0bf734eaa55da9c58b0a
>
>
> Diff: https://reviews.apache.org/r/57167/diff/1/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Neil Conway
>
>