mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Meng Zhu <m...@mesosphere.io>
Subject Re: Review Request 70707: Refactored role quota info into `Struct Role`.
Date Mon, 27 May 2019 12:17:16 GMT


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


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