mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Till Toenshoff <toensh...@me.com>
Subject Re: Review Request 65939: Updated role endpoints for hierarchical accounting.
Date Tue, 10 Apr 2018 02:57:45 GMT


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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message