You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Benjamin Mahler <bm...@apache.org> on 2019/07/15 20:05:23 UTC
Review Request 71073: Fixed /roles and GET_ROLES to expose all known
roles.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71073/
-----------------------------------------------------------
Review request for mesos, Andrei Sekretenko and Meng Zhu.
Bugs: MESOS-9888 and MESOS-9890
https://issues.apache.org/jira/browse/MESOS-9888
https://issues.apache.org/jira/browse/MESOS-9890
Repository: mesos
Description
-------
Previously, per MESOS-9888 and MESOS-9890, the /roles and GET_ROLES
APIs only exposed roles that had frameworks associated with them
(either because the framework is subscribed to the role, or there
is a framework with allocations to the role) or configured weight
and/or quota.
This approach omits some important cases:
(1) Roles that have only reservations associated with them.
(2) Roles that have only a parent relationship to other roles.
This patch exposes a function that returns all "known" roles based
on the criteria we care about:
(1) Roles with configured weight or quota.
(2) Roles with reservations.
(3) Roles with frameworks subscribed or allocated resources.
(4) Ancestor roles of (1), (2), or (3).
Also, the resource breakdowns are pulled out from the Role struct
and placed in a function that returns the breakdowns for all known
roles. This was done because there is currently not a Role struct
entry for all known roles.
Diffs
-----
src/master/http.cpp cd0f40cb7b966d6620e3fb49d4c08807185c9101
src/master/master.hpp e8def83fe9bcee19772df9a9764852bc694c5247
src/master/master.cpp 5247377c2e7e92b9843dd4c9d28f92ba679ad742
src/master/readonly_handler.cpp f4432a54a21134192636b76a5c36d0241e10dabc
Diff: https://reviews.apache.org/r/71073/diff/1/
Testing
-------
Updated a test.
Thanks,
Benjamin Mahler
Re: Review Request 71073: Fixed /roles and GET_ROLES to expose all
known roles.
Posted by Benjamin Mahler <bm...@apache.org>.
> On July 15, 2019, 9:37 p.m., Meng Zhu wrote:
> > Thanks for fixing this!
> >
> > The indentation of the bullet points in the commit message is a bit off.
Seems to be reviewboard's rendering.
> On July 15, 2019, 9:37 p.m., Meng Zhu wrote:
> > src/master/master.cpp
> > Lines 3531-3533 (original), 3520-3522 (patched)
> > <https://reviews.apache.org/r/71073/diff/1/?file=2154910#file2154910line3531>
> >
> > It should be more efficient to use unordered_set and then sort afterward.
> >
> > Not sure how much difference would it make, given the function is already expensive.
Will it? It doesn't seem that obvious to me that it will be faster, but I can't easily measure now.
> On July 15, 2019, 9:37 p.m., Meng Zhu wrote:
> > src/master/master.cpp
> > Lines 3537 (patched)
> > <https://reviews.apache.org/r/71073/diff/1/?file=2154910#file2154910line3548>
> >
> > not needed here?
As discussed offline, removing this would make it inconsistent about when ancestors are exposed. E.g. you wouldn't be able to necessarily see "eng" when "eng/frontend" and "eng/backend" entries are present, which seems unfortunate for the user and for the UI to try to deal with.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71073/#review216617
-----------------------------------------------------------
On July 15, 2019, 8:05 p.m., Benjamin Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71073/
> -----------------------------------------------------------
>
> (Updated July 15, 2019, 8:05 p.m.)
>
>
> Review request for mesos, Andrei Sekretenko and Meng Zhu.
>
>
> Bugs: MESOS-9888 and MESOS-9890
> https://issues.apache.org/jira/browse/MESOS-9888
> https://issues.apache.org/jira/browse/MESOS-9890
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Previously, per MESOS-9888 and MESOS-9890, the /roles and GET_ROLES
> APIs only exposed roles that had frameworks associated with them
> (either because the framework is subscribed to the role, or there
> is a framework with allocations to the role) or configured weight
> and/or quota.
>
> This approach omits some important cases:
>
> (1) Roles that have only reservations associated with them.
> (2) Roles that have only a parent relationship to other roles.
>
> This patch exposes a function that returns all "known" roles based
> on the criteria we care about:
>
> (1) Roles with configured weight or quota.
> (2) Roles with reservations.
> (3) Roles with frameworks subscribed or allocated resources.
> (4) Ancestor roles of (1), (2), or (3).
>
> Also, the resource breakdowns are pulled out from the Role struct
> and placed in a function that returns the breakdowns for all known
> roles. This was done because there is currently not a Role struct
> entry for all known roles.
>
>
> Diffs
> -----
>
> src/master/http.cpp cd0f40cb7b966d6620e3fb49d4c08807185c9101
> src/master/master.hpp e8def83fe9bcee19772df9a9764852bc694c5247
> src/master/master.cpp 5247377c2e7e92b9843dd4c9d28f92ba679ad742
> src/master/readonly_handler.cpp f4432a54a21134192636b76a5c36d0241e10dabc
>
>
> Diff: https://reviews.apache.org/r/71073/diff/1/
>
>
> Testing
> -------
>
> Updated a test.
>
>
> Thanks,
>
> Benjamin Mahler
>
>
Re: Review Request 71073: Fixed /roles and GET_ROLES to expose all
known roles.
Posted by Meng Zhu <mz...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71073/#review216617
-----------------------------------------------------------
Fix it, then Ship it!
Thanks for fixing this!
The indentation of the bullet points in the commit message is a bit off.
src/master/master.cpp
Lines 3531-3533 (original), 3520-3522 (patched)
<https://reviews.apache.org/r/71073/#comment303830>
It should be more efficient to use unordered_set and then sort afterward.
Not sure how much difference would it make, given the function is already expensive.
src/master/master.cpp
Lines 3537 (patched)
<https://reviews.apache.org/r/71073/#comment303831>
not needed here?
src/master/master.cpp
Line 3538 (original), 3540 (patched)
<https://reviews.apache.org/r/71073/#comment303832>
s/tree/set or list/ ?
src/master/master.cpp
Lines 3555-3557 (original), 3570-3577 (patched)
<https://reviews.apache.org/r/71073/#comment303829>
can be simplified to:
```
return vector<string>(roleList.begin(), roleList.end());
```
- Meng Zhu
On July 15, 2019, 1:05 p.m., Benjamin Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71073/
> -----------------------------------------------------------
>
> (Updated July 15, 2019, 1:05 p.m.)
>
>
> Review request for mesos, Andrei Sekretenko and Meng Zhu.
>
>
> Bugs: MESOS-9888 and MESOS-9890
> https://issues.apache.org/jira/browse/MESOS-9888
> https://issues.apache.org/jira/browse/MESOS-9890
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Previously, per MESOS-9888 and MESOS-9890, the /roles and GET_ROLES
> APIs only exposed roles that had frameworks associated with them
> (either because the framework is subscribed to the role, or there
> is a framework with allocations to the role) or configured weight
> and/or quota.
>
> This approach omits some important cases:
>
> (1) Roles that have only reservations associated with them.
> (2) Roles that have only a parent relationship to other roles.
>
> This patch exposes a function that returns all "known" roles based
> on the criteria we care about:
>
> (1) Roles with configured weight or quota.
> (2) Roles with reservations.
> (3) Roles with frameworks subscribed or allocated resources.
> (4) Ancestor roles of (1), (2), or (3).
>
> Also, the resource breakdowns are pulled out from the Role struct
> and placed in a function that returns the breakdowns for all known
> roles. This was done because there is currently not a Role struct
> entry for all known roles.
>
>
> Diffs
> -----
>
> src/master/http.cpp cd0f40cb7b966d6620e3fb49d4c08807185c9101
> src/master/master.hpp e8def83fe9bcee19772df9a9764852bc694c5247
> src/master/master.cpp 5247377c2e7e92b9843dd4c9d28f92ba679ad742
> src/master/readonly_handler.cpp f4432a54a21134192636b76a5c36d0241e10dabc
>
>
> Diff: https://reviews.apache.org/r/71073/diff/1/
>
>
> Testing
> -------
>
> Updated a test.
>
>
> Thanks,
>
> Benjamin Mahler
>
>