> On April 6, 2018, 12:25 a.m., Kapil Arya wrote:
> > src/master/master.hpp
> > Lines 139-140 (patched)
> > <https://reviews.apache.org/r/65939/diff/2/?file=1993040#file1993040line140>
> >
> > Now that we are keeping track of `allocatedResources` and
> > `totalAllocatedResources`, we can replace this hashmap with a simple
> > hashset. We don't seem to be using `Framework*` elsewhere.
There is more uses of the framework pointer.
See e.g. `quota_handler.cpp:259`
```
foreachvalue (const Framework* framework, roleState->frameworks) {
if (framework->active()) {
++frameworksInRole;
}
}
```
However, it seems we can replace that after following your advice:
```
foreach (const FrameworkID& frameworkID, roleState->frameworks) {
CHECK(master->frameworks.registered.contains(frameworkID));
Framework* framework = master->frameworks.registered.at(frameworkID);
if (framework->active()) {
++frameworksInRole;
}
}
```
Seems that the price for that simplification is not that high.
> On April 6, 2018, 12:25 a.m., Kapil Arya wrote:
> > src/master/master.hpp
> > Lines 178 (patched)
> > <https://reviews.apache.org/r/65939/diff/2/?file=1993040#file1993040line179>
> >
> > I am not sure if I understand what `returned in ["a", "a/b"]` means.
New version:
```
// For example: Single framework registered with the role "a/b". That
// makes "a/b" an active role. The parent role "a" of "a/b" gets
// implicitly tracked. This function would return two roles; "a" and
// "a/b".
```
> On April 6, 2018, 12:25 a.m., Kapil Arya wrote:
> > src/master/master.hpp
> > Lines 181 (patched)
> > <https://reviews.apache.org/r/65939/diff/2/?file=1993040#file1993040line182>
> >
> > Would this search the framework in child roles as well? Can we expand
> > to comment to explicitly mention it? Something like:
> >
> > ` Checks if the given framework is being tracked under the given role
> > or its child roles`
The implementation is this
```
bool RoleTrackingTree::hasFramework(
const string& role,
const FrameworkID& id) const
{
return hasRole(role) && roleMap.at(role).frameworks.contains(id);
}
```
That means it checks `frameworks` and not `total_frameworks`. The latter is the
aggregated value containing frameworks of child-roles.
> On April 6, 2018, 12:25 a.m., Kapil Arya wrote:
> > src/master/master.cpp
> > Lines 531 (patched)
> > <https://reviews.apache.org/r/65939/diff/2/?file=1993041#file1993041line535>
> >
> > s/childless/leaf ?
I removed "childless".
> On April 6, 2018, 12:25 a.m., Kapil Arya wrote:
> > src/master/master.cpp
> > Lines 572 (patched)
> > <https://reviews.apache.org/r/65939/diff/2/?file=1993041#file1993041line576>
> >
> > Wondering if we should use `getRole(role)->frameworks.contains(id)` for
> > uniformity.
getRole() is not const - calling would render `hasFramework` non const.
> On April 6, 2018, 12:25 a.m., Kapil Arya wrote:
> > src/master/master.cpp
> > Lines 587 (patched)
> > <https://reviews.apache.org/r/65939/diff/2/?file=1993041#file1993041line591>
> >
> > Do we need a CHECK here, or is that too defensive?
We technically don't need it as `erase` wont throw on an `unordered_set`.
However it is nice to have nevertheless :)
> On April 6, 2018, 12:25 a.m., Kapil Arya wrote:
> > src/master/master.cpp
> > Lines 619 (patched)
> > <https://reviews.apache.org/r/65939/diff/2/?file=1993041#file1993041line623>
> >
> > This VLOG seems to stand out. Do we need similar log messages in other
> > functions (`addResources`, etc.)?
Indeed a debugging artefact - ty.
- Till
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65939/#review200584
-----------------------------------------------------------
On April 5, 2018, 4:15 p.m., Till Toenshoff wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65939/
> -----------------------------------------------------------
>
> (Updated April 5, 2018, 4:15 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, Kapil Arya,
> Michael Park, and Meng Zhu.
>
>
> Bugs: MESOS-8069
> https://issues.apache.org/jira/browse/MESOS-8069
>
>
> Repository: mesos
>
>
> Description
> -------
>
> When listing role "a", resources allocated to role "a/b" are added to
> those allocated to role "a". These changes affect both, the "/roles"
> endpoint as well as the V1 HTTP Call "GET_ROLES".
> Adds dynamic hierarchical role tracking to the master.
>
>
> Diffs
> -----
>
> include/mesos/master/master.proto aa63904a33290a3beda162bbc9f44b56ab04a1e7
> include/mesos/mesos.proto 676f0b090cad7ebf59eb32556f17ff8b5f247907
> include/mesos/v1/master/master.proto
> ddb28f96b2a3a439bb9a829995a9a3015f65ba43
> include/mesos/v1/mesos.proto 10d506517c9f098374ab0c8f4dcfda42e1896c95
> src/master/http.cpp 34c9023906eca94965acc994f20e888c1f47b962
> src/master/master.hpp 0d9620dd0c232dc1df83477e838eeb7313bf8828
> src/master/master.cpp 18382fa93fd0c59e641e00f2028ac1ae2e67c01c
> src/master/quota_handler.cpp 21bafd0064e9397f88185eaf687a58f85da94e2c
> src/master/weights_handler.cpp 1053652804a8fc6af31a3b8a9f632f836c897fa9
> src/tests/api_tests.cpp dd8e221d8fd1b2a241505345337897e4ee4a6347
> src/tests/role_tests.cpp a609ed27ef828ca82efc0d269669cda92e5f50a1
>
>
> Diff: https://reviews.apache.org/r/65939/diff/3/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Till Toenshoff
>
>
