Re: Review Request 57167: Updated quota handler logic for hierarchical roles.

[Michael Park]

> On March 8, 2017, 10:38 a.m., Michael Park wrote:
> > src/master/quota_handler.cpp
> > Lines 70 (patched)
> > 
> >
> > What do you think of introducing a `QuotaTree` with which we can 
> > encapsulate all this stuff?
> > 
> > Something like:
> > 
> > ```cpp
> > class QuotaTree {
> >   QuotaTree() = default;
> > 
> >   Try insert(const string& role, const Quota& quota) const {
> > // basically the body of `foreachpair` in `buildQuotaTree` + 
> > validation.
> >   }
> >   
> >   Resources total() const;
> > 
> >   private:
> >   
> >   class Node { ... };
> >   
> >   unique_ptr root;
> > };
> > 
> > Try buildQuotaTree(const hashmap& quotas)
> > {
> >   QuotaTree result;
> > 
> >   foreachpair (cons string& role, const Quota& quota, quotas) {
> > Try 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&);
> >   Option 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.

[Neil Conway]

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

[Michael Park]

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


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.

[Neil Conway]

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

[Neil Conway]

> On March 28, 2017, 11:06 p.m., Michael Park wrote:
> > src/master/quota_handler.cpp
> > Lines 469-487 (original), 605-618 (patched)
> > 
> >
> > Couldn't this be just:
> > 
> > ```cpp
> > vector 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.

[Michael Park]

> On March 28, 2017, 4:06 p.m., Michael Park wrote:
> > src/master/quota_handler.cpp
> > Lines 469-487 (original), 605-618 (patched)
> > 
> >
> > Couldn't this be just:
> > 
> > ```cpp
> > vector 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.

[Neil Conway]

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

[Neil Conway]

> On March 28, 2017, 11:06 p.m., Michael Park wrote:
> > src/master/quota_handler.cpp
> > Lines 111-115 (patched)
> > 
> >
> > 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)
> > 
> >
> > Couldn't this be just:
> > 
> > ```cpp
> > vector 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.

[Michael Park]

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


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)


Let's still put the members at the bottom.



src/master/quota_handler.cpp
Lines 469-487 (original), 605-618 (patched)


Couldn't this be just:

```cpp
vector 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)


How about something like this?
```cpp
map expected = {{parent.role(), parent.guarantee()},
   {child.role(), child.guarantee()}};

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

[Neil Conway]

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

[Neil Conway]

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

[Neil Conway]

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

[Jay Guo]

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

[Jay Guo]

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


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.

[Michael Park]

> On March 8, 2017, 10:38 a.m., Michael Park wrote:
> > src/master/quota_handler.cpp
> > Lines 78 (patched)
> > 
> >
> > The general pattern for `validate` I think is to return an 
> > `Option`. In this case, we could more accurately report what/where 
> > the invalid relationship is within the tree.
> 
> Neil Conway wrote:
> `Option` 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 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& child, root->children) {
if (!child->isValid()) {
  return false;
}
  }
  
  return true;
}
```

to...

```cpp
Option validate() const
{
  foreachvalue (const unique_ptr& child, root->children) {
Option validate = child->validate();
if (validate.isSome()) {
  return validate;
}
  }
  
  return None();
}
```

and,

```cpp
bool isValid() const
{
  foreachvalue (const unique_ptr& child, children) {
if (!child->isValid()) {
  return false;
}
  }

  Resources childResources;
  foreachvalue (const unique_ptr& child, children) {
childResources += child->quota.info.guarantee();
  }

  Resources selfResources = quota.info.guarantee();

  return selfResources.contains(childResources);
}
```

to...

```cpp
Option validate() const
{
  foreachvalue (const unique_ptr& child, children) {
Option validate = child->validate();
if (validate.isSome()) {
  return validate;
}
  }

  Resources childResources;
  foreachvalue (const unique_ptr& 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.

[Neil Conway]

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

[Neil Conway]

> On March 8, 2017, 6:38 p.m., Michael Park wrote:
> > src/master/quota_handler.cpp
> > Lines 78 (patched)
> > 
> >
> > The general pattern for `validate` I think is to return an 
> > `Option`. In this case, we could more accurately report what/where 
> > the invalid relationship is within the tree.

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

[Benjamin Mahler]

> On March 9, 2017, 9:39 p.m., Michael Park wrote:
> > src/master/quota_handler.cpp
> > Lines 469-487 (original), 583-605 (patched)
> > 
> >
> > 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.

[Michael Park]

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


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)


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.

[Neil Conway]

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

[Neil Conway]

> On March 9, 2017, 9:39 p.m., Michael Park wrote:
> > src/master/quota_handler.cpp
> > Lines 469-487 (original), 583-605 (patched)
> > 
> >
> > 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.

[Michael Park]

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


Why not just make this in the constructor?



src/master/quota_handler.cpp
Lines 469-487 (original), 583-605 (patched)


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.

[Neil Conway]

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

[Neil Conway]

> On March 8, 2017, 6:38 p.m., Michael Park wrote:
> > src/master/quota_handler.cpp
> > Lines 70 (patched)
> > 
> >
> > What do you think of introducing a `QuotaTree` with which we can 
> > encapsulate all this stuff?
> > 
> > Something like:
> > 
> > ```cpp
> > class QuotaTree {
> >   QuotaTree() = default;
> > 
> >   Try insert(const string& role, const Quota& quota) const {
> > // basically the body of `foreachpair` in `buildQuotaTree` + 
> > validation.
> >   }
> >   
> >   Resources total() const;
> > 
> >   private:
> >   
> >   class Node { ... };
> >   
> >   unique_ptr root;
> > };
> > 
> > Try buildQuotaTree(const hashmap& quotas)
> > {
> >   QuotaTree result;
> > 
> >   foreachpair (cons string& role, const Quota& quota, quotas) {
> > Try 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&);
> >   Option 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.

[Michael Park]

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


What do you think of introducing a `QuotaTree` with which we can 
encapsulate all this stuff?

Something like:

```cpp
class QuotaTree {
  QuotaTree() = default;

  Try insert(const string& role, const Quota& quota) const {
// basically the body of `foreachpair` in `buildQuotaTree` + validation.
  }
  
  Resources total() const;

  private:
  
  class Node { ... };
  
  unique_ptr root;
};

Try buildQuotaTree(const hashmap& quotas)
{
  QuotaTree result;

  foreachpair (cons string& role, const Quota& quota, quotas) {
Try 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&);
  Option validate() const;
  Resources total() const;
  
  // private stuff...
};
```

could simplify the usage a little bit.



src/master/quota_handler.cpp
Lines 78 (patched)


The general pattern for `validate` I think is to return an `Option`. 
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)


We can just use the `Resources` implicit constructor here.
```cpp
childResources += child->quota.info.guarantee()
```



src/master/quota_handler.cpp
Lines 92-95 (patched)


Ditto.
```cpp
selfResources = quota.info.guarantee()
```



src/master/quota_handler.cpp
Lines 102 (patched)


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

Re: Review Request 57167: Updated quota handler logic for hierarchical roles.

[Mesos Reviewbot]

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

Review Request 57167: Updated quota handler logic for hierarchical roles.

[Neil Conway]

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