You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Meng Zhu <mz...@mesosphere.io> on 2019/05/23 14:48:58 UTC
Review Request 70707: Refactored role quota info into `Struct Role`.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70707/
-----------------------------------------------------------
Review request for mesos, Andrei Sekretenko and Benjamin Mahler.
Repository: mesos
Description
-------
This avoids an extra map of tracking role quota info.
Diffs
-----
src/master/allocator/mesos/hierarchical.hpp 7e4f30d16010a777ff29ba35366ae80ff2631322
src/master/allocator/mesos/hierarchical.cpp ccca391fbf73d9aa9c31f7552e39fb78c242e242
Diff: https://reviews.apache.org/r/70707/diff/1/
Testing
-------
make check
Thanks,
Meng Zhu
Re: Review Request 70707: Refactored role quota info into `Struct
Role`.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70707/#review215488
-----------------------------------------------------------
Patch looks great!
Reviews applied: [70705, 70706, 70707]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose --disable-libtool-wrappers --disable-parallel-test-execution' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On May 23, 2019, 2:48 p.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70707/
> -----------------------------------------------------------
>
> (Updated May 23, 2019, 2:48 p.m.)
>
>
> Review request for mesos, Andrei Sekretenko and Benjamin Mahler.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This avoids an extra map of tracking role quota info.
>
>
> Diffs
> -----
>
> src/master/allocator/mesos/hierarchical.hpp 7e4f30d16010a777ff29ba35366ae80ff2631322
> src/master/allocator/mesos/hierarchical.cpp ccca391fbf73d9aa9c31f7552e39fb78c242e242
>
>
> Diff: https://reviews.apache.org/r/70707/diff/1/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Meng Zhu
>
>
Re: Review Request 70707: Refactored role quota info into `Struct
Role`.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70707/#review215581
-----------------------------------------------------------
Ship it!
src/master/allocator/mesos/hierarchical.hpp
Lines 120-122 (patched)
<https://reviews.apache.org/r/70707/#comment302319>
See the struct suggestion below
src/master/allocator/mesos/hierarchical.hpp
Lines 620-627 (patched)
<https://reviews.apache.org/r/70707/#comment302318>
Should this be a constant in constants.hpp?
E.g. DEFAULT_QUOTA_GUARANTEES
src/master/allocator/mesos/hierarchical.hpp
Lines 634 (patched)
<https://reviews.apache.org/r/70707/#comment302317>
struct Quota
{
ResourceQuantities guarantees;
ResourceLimits limits;
}
Quota getQuota(role);
Can we add a struct to group the C++ types together in general for the master and allocator?
src/master/allocator/mesos/hierarchical.cpp
Line 1412 (original), 1412 (patched)
<https://reviews.apache.org/r/70707/#comment302320>
Maybe check it's the default?
CHECK_EQ(defaultQuota, getQuota(role));
src/master/allocator/mesos/hierarchical.cpp
Line 1440 (original), 1440 (patched)
<https://reviews.apache.org/r/70707/#comment302321>
Ditto here
src/master/allocator/mesos/hierarchical.cpp
Lines 1443-1444 (original), 1445-1446 (patched)
<https://reviews.apache.org/r/70707/#comment302322>
Not yours but for readability:
```
LOG(INFO) << "Removed quota " << r.quotaGuarantee <<
<< " for role '" << role << "'";
```
src/master/allocator/mesos/hierarchical.cpp
Line 1674 (original), 1686 (patched)
<https://reviews.apache.org/r/70707/#comment302323>
r.quota.guarantees.empty() ?
src/master/allocator/mesos/hierarchical.cpp
Line 1690 (original), 1703 (patched)
<https://reviews.apache.org/r/70707/#comment302324>
Same here? Can use the role struct we already have in hand?
- Benjamin Mahler
On May 29, 2019, 3:22 p.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70707/
> -----------------------------------------------------------
>
> (Updated May 29, 2019, 3:22 p.m.)
>
>
> Review request for mesos, Andrei Sekretenko and Benjamin Mahler.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This avoids an extra map of tracking role quota info.
> Also added a helper `getGuarantees()` to look up quota guarantees
> of a given role.
>
>
> Diffs
> -----
>
> src/master/allocator/mesos/hierarchical.hpp 71a9656fb934bf9ac58e3165254ea49cb09efa8b
> src/master/allocator/mesos/hierarchical.cpp 40c8363afddccdd5275ca06318a8cc2cc6fa21af
>
>
> Diff: https://reviews.apache.org/r/70707/diff/4/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Meng Zhu
>
>
Re: Review Request 70707: Refactored role quota info into `Struct
Role`.
Posted by Meng Zhu <mz...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70707/
-----------------------------------------------------------
(Updated May 29, 2019, 8:22 a.m.)
Review request for mesos, Andrei Sekretenko and Benjamin Mahler.
Changes
-------
Added a helper `getGuarantees()` to look up quota guarantees of a given role.
Repository: mesos
Description (updated)
-------
This avoids an extra map of tracking role quota info.
Also added a helper `getGuarantees()` to look up quota guarantees
of a given role.
Diffs (updated)
-----
src/master/allocator/mesos/hierarchical.hpp 71a9656fb934bf9ac58e3165254ea49cb09efa8b
src/master/allocator/mesos/hierarchical.cpp 40c8363afddccdd5275ca06318a8cc2cc6fa21af
Diff: https://reviews.apache.org/r/70707/diff/4/
Changes: https://reviews.apache.org/r/70707/diff/3-4/
Testing
-------
make check
Thanks,
Meng Zhu
Re: Review Request 70707: Refactored role quota info into `Struct
Role`.
Posted by Meng Zhu <mz...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70707/
-----------------------------------------------------------
(Updated May 28, 2019, 4:50 a.m.)
Review request for mesos, Andrei Sekretenko and Benjamin Mahler.
Changes
-------
Addressed Ben's comment.
Repository: mesos
Description
-------
This avoids an extra map of tracking role quota info.
Diffs (updated)
-----
src/master/allocator/mesos/hierarchical.hpp 71a9656fb934bf9ac58e3165254ea49cb09efa8b
src/master/allocator/mesos/hierarchical.cpp 40c8363afddccdd5275ca06318a8cc2cc6fa21af
Diff: https://reviews.apache.org/r/70707/diff/3/
Changes: https://reviews.apache.org/r/70707/diff/2-3/
Testing
-------
make check
Thanks,
Meng Zhu
Re: Review Request 70707: Refactored role quota info into `Struct
Role`.
Posted by Meng Zhu <mz...@mesosphere.io>.
> On May 24, 2019, 4:57 p.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.hpp
> > Line 123 (original), 125-126 (patched)
> > <https://reviews.apache.org/r/70707/diff/2/?file=2146621#file2146621line127>
> >
> > just one on each line at this point seems more readable?
Sounds good.
> On May 24, 2019, 4:57 p.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.hpp
> > Lines 644-645 (patched)
> > <https://reviews.apache.org/r/70707/diff/2/?file=2146621#file2146621line651>
> >
> > "Has quota" seems inaccurate and will break down soon when we introduce the ability to set the limits.
> >
> > Strictly speaking, every role has a quota, it just might be 0 guarantee / no limit (default).
> >
> > Can't this be a property of the role struct? Looking at code like this:
> >
> > ```
> > if (roleHasQuota(role)) {
> > ```
> >
> > Makes me think the code should just use the role struct:
> >
> > ```
> > Role r = roles.at(role);
> >
> > if (r.quota.guarantee > ResourceQuantities()) {
> > ...
> > }
> > ```
> "Has quota" seems inaccurate
I could name it `hasQuotaSet`
> will break down soon when we introduce the ability to set the limits
why? We could just need to add the limit check as well
return roles.contains(role) && (!roles.at(role).quotaGuarantees.empty() || ! roles.at(role).quotaLimits.empty())
> Can't this be a property of the role struct?
It certainly could. But (1) it is very verbose and hurts readability; (2) See my answer below.
> On May 24, 2019, 4:57 p.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp
> > Lines 2558 (patched)
> > <https://reviews.apache.org/r/70707/diff/2/?file=2146622#file2146622line2561>
> >
> > Per my above comment, when would roles not contain role when we're asking about whether a role has quota?
This depends on "when we're asking" :) While most of the time a role struct would exist, it is not always true and this is what makes the check necessary. Just a couple of examples:
- https://github.com/apache/mesos/blob/master/src/master/allocator/mesos/hierarchical.cpp#L1436
Change this line to `CHECK(!roleHasQuota(role));`
Here, a role may not exist if the quota info is all it has atm.
- https://github.com/apache/mesos/blob/264a27ae1c9f7fc587c3c7daf60025c11c98305f/src/master/allocator/mesos/hierarchical.cpp#L1731
I think top-level roles may not exist despite the existence of sub-level roles
I think without a complete role life cycle management, it is really hard to say when a role exists or not. Until we add life cycle management, I suggest let's do the existence check for the hasQuotaSet call.
What do you think?
- Meng
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70707/#review215518
-----------------------------------------------------------
On May 24, 2019, 4:52 a.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70707/
> -----------------------------------------------------------
>
> (Updated May 24, 2019, 4:52 a.m.)
>
>
> Review request for mesos, Andrei Sekretenko and Benjamin Mahler.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This avoids an extra map of tracking role quota info.
>
>
> Diffs
> -----
>
> src/master/allocator/mesos/hierarchical.hpp 7e4f30d16010a777ff29ba35366ae80ff2631322
> src/master/allocator/mesos/hierarchical.cpp ccca391fbf73d9aa9c31f7552e39fb78c242e242
>
>
> Diff: https://reviews.apache.org/r/70707/diff/2/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Meng Zhu
>
>
Re: Review Request 70707: Refactored role quota info into `Struct
Role`.
Posted by Meng Zhu <mz...@mesosphere.io>.
> On May 24, 2019, 4:57 p.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.hpp
> > Lines 644-645 (patched)
> > <https://reviews.apache.org/r/70707/diff/2/?file=2146621#file2146621line651>
> >
> > "Has quota" seems inaccurate and will break down soon when we introduce the ability to set the limits.
> >
> > Strictly speaking, every role has a quota, it just might be 0 guarantee / no limit (default).
> >
> > Can't this be a property of the role struct? Looking at code like this:
> >
> > ```
> > if (roleHasQuota(role)) {
> > ```
> >
> > Makes me think the code should just use the role struct:
> >
> > ```
> > Role r = roles.at(role);
> >
> > if (r.quota.guarantee > ResourceQuantities()) {
> > ...
> > }
> > ```
>
> Meng Zhu wrote:
> > "Has quota" seems inaccurate
>
> I could name it `hasQuotaSet`
>
> > will break down soon when we introduce the ability to set the limits
> why? We could just need to add the limit check as well
>
> return roles.contains(role) && (!roles.at(role).quotaGuarantees.empty() || ! roles.at(role).quotaLimits.empty())
>
>
> > Can't this be a property of the role struct?
> It certainly could. But (1) it is very verbose and hurts readability; (2) See my answer below.
Introduced a helper `getGuarantees()` to look up quota guarantees of a given role, it would return an empty ResourceQuantity object if the role only has default guarantees.
- Meng
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70707/#review215518
-----------------------------------------------------------
On May 29, 2019, 8:22 a.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70707/
> -----------------------------------------------------------
>
> (Updated May 29, 2019, 8:22 a.m.)
>
>
> Review request for mesos, Andrei Sekretenko and Benjamin Mahler.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This avoids an extra map of tracking role quota info.
> Also added a helper `getGuarantees()` to look up quota guarantees
> of a given role.
>
>
> Diffs
> -----
>
> src/master/allocator/mesos/hierarchical.hpp 71a9656fb934bf9ac58e3165254ea49cb09efa8b
> src/master/allocator/mesos/hierarchical.cpp 40c8363afddccdd5275ca06318a8cc2cc6fa21af
>
>
> Diff: https://reviews.apache.org/r/70707/diff/4/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Meng Zhu
>
>
Re: Review Request 70707: Refactored role quota info into `Struct
Role`.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70707/#review215518
-----------------------------------------------------------
src/master/allocator/mesos/hierarchical.hpp
Line 123 (original), 125-126 (patched)
<https://reviews.apache.org/r/70707/#comment302252>
just one on each line at this point seems more readable?
src/master/allocator/mesos/hierarchical.hpp
Lines 644-645 (patched)
<https://reviews.apache.org/r/70707/#comment302253>
"Has quota" seems inaccurate and will break down soon when we introduce the ability to set the limits.
Strictly speaking, every role has a quota, it just might be 0 guarantee / no limit (default).
Can't this be a property of the role struct? Looking at code like this:
```
if (roleHasQuota(role)) {
```
Makes me think the code should just use the role struct:
```
Role r = roles.at(role);
if (r.quota.guarantee > ResourceQuantities()) {
...
}
```
src/master/allocator/mesos/hierarchical.cpp
Lines 2558 (patched)
<https://reviews.apache.org/r/70707/#comment302254>
Per my above comment, when would roles not contain role when we're asking about whether a role has quota?
- Benjamin Mahler
On May 24, 2019, 11:52 a.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70707/
> -----------------------------------------------------------
>
> (Updated May 24, 2019, 11:52 a.m.)
>
>
> Review request for mesos, Andrei Sekretenko and Benjamin Mahler.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This avoids an extra map of tracking role quota info.
>
>
> Diffs
> -----
>
> src/master/allocator/mesos/hierarchical.hpp 7e4f30d16010a777ff29ba35366ae80ff2631322
> src/master/allocator/mesos/hierarchical.cpp ccca391fbf73d9aa9c31f7552e39fb78c242e242
>
>
> Diff: https://reviews.apache.org/r/70707/diff/2/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Meng Zhu
>
>
Re: Review Request 70707: Refactored role quota info into `Struct
Role`.
Posted by Meng Zhu <mz...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70707/
-----------------------------------------------------------
(Updated May 24, 2019, 4:52 a.m.)
Review request for mesos, Andrei Sekretenko and Benjamin Mahler.
Changes
-------
Removed the option wrapper.
Repository: mesos
Description
-------
This avoids an extra map of tracking role quota info.
Diffs (updated)
-----
src/master/allocator/mesos/hierarchical.hpp 7e4f30d16010a777ff29ba35366ae80ff2631322
src/master/allocator/mesos/hierarchical.cpp ccca391fbf73d9aa9c31f7552e39fb78c242e242
Diff: https://reviews.apache.org/r/70707/diff/2/
Changes: https://reviews.apache.org/r/70707/diff/1-2/
Testing
-------
make check
Thanks,
Meng Zhu