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