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