mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Mahler <bmah...@apache.org>
Subject Re: Review Request 70276: Included RP resources / static reservations in quota capacity check.
Date Mon, 25 Mar 2019 21:42:18 GMT


> On March 25, 2019, 12:06 a.m., Meng Zhu wrote:
> > src/master/master.hpp
> > Lines 1195-1199 (original), 1195-1199 (patched)
> > <https://reviews.apache.org/r/70276/diff/1/?file=2133529#file2133529line1195>
> >
> >     Why cut the comments here? I think we should at least mention what we are checking
i.e. include a formula like that in the old code.
> >     
> >     In addition, since we remove the static reservation check, it would be nice
if we can include a comment here regarding reservations (both static and dynamic) for people
who wonders about the old behavior.

I tried to distill it, I can certainly add back an inequality formula. I can also add some
commentary on reservations.

Is there anything else you specficially think should be called out in the comment?


> On March 25, 2019, 12:06 a.m., Meng Zhu wrote:
> > src/master/quota_handler.cpp
> > Lines 222-224 (original), 217-219 (patched)
> > <https://reviews.apache.org/r/70276/diff/1/?file=2133530#file2133530line223>
> >
> >     Early termination is still beneficial. We could also do the subtraction optimization
I mentioned in the previous review.
> >     
> >     Also, we are mixing the behavior change and some refactoring here. For clarity,
it will be great if we can separate them into different patches.

I assume the behavior + refactoring mixing is related to your other comment below:

> Excluding revocable resources is another change of behavior here. Let's mention that
in the description and comment.

If so, see my comment there. If not, let me know if there appears to be another behavioral
change

> Early termination is still beneficial. We could also do the subtraction optimization
I mentioned in the previous review.

Hm.. it depends on the cost of the early termination check as well as at which point the loop
will terminate. It will make worst-case performance worse, technically? Can we consider this
more deeply later if we find that this is actually a significant portion of the time spent?
My hunch is it won't be and that other aspects will be more important (e.g. execute the quota
handler logic outside of the master actor, avoid having to re-build quota trees which entails
lots of string copying and heap allocations, etc).


> On March 25, 2019, 12:06 a.m., Meng Zhu wrote:
> > src/master/quota_handler.cpp
> > Line 224 (original), 219 (patched)
> > <https://reviews.apache.org/r/70276/diff/1/?file=2133530#file2133530line225>
> >
> >     Excluding revocable resources is another change of behavior here. Let's mention
that in the description and comment.

It's a bit subtle but it's not a behavioral change, we have to now exclude them since the
argument now includes them.

The old argument was SlaveInfo.resources, which does not include the additional revocable
resources. (i.e. it was not the actual agent total resources)


> On March 25, 2019, 12:06 a.m., Meng Zhu wrote:
> > src/master/quota_handler.cpp
> > Line 617 (original), 602 (patched)
> > <https://reviews.apache.org/r/70276/diff/1/?file=2133530#file2133530line621>
> >
> >     We could just pass the quantities here?

Hm... do you have a more concrete suggestion? Doing so requires filtering out resources here
(e.g. non revocable only) and it seemed to make more sense to just pass in the total and have
the overcommit function be responsible for looking at the appropriate resources within the
overall total (i.e. the non revocable subset)


- Benjamin


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70276/#review213959
-----------------------------------------------------------


On March 22, 2019, 3:54 p.m., Benjamin Mahler wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70276/
> -----------------------------------------------------------
> 
> (Updated March 22, 2019, 3:54 p.m.)
> 
> 
> Review request for mesos and Meng Zhu.
> 
> 
> Bugs: MESOS-7883
>     https://issues.apache.org/jira/browse/MESOS-7883
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Static reservations and resource provider resources both count towards
> quota consumption and should therefore be included in the quota
> capacity check.
> 
> Note also that this renames the function to be an "overcommit" check
> rather than making any suggestions of being a "satisfiability" check.
> The latter is far more complex to implement properly, and the current
> quota API design includes a simple overcommit check rather than any
> attempt as satisfiability checking.
> 
> 
> Diffs
> -----
> 
>   src/master/master.hpp 77be494d1ca2d3cd79ab033e41a5ff66c0225f79 
>   src/master/quota_handler.cpp 2dcfdcaf836f21d7d39b5ef5c36de0db25ca7517 
> 
> 
> Diff: https://reviews.apache.org/r/70276/diff/1/
> 
> 
> Testing
> -------
> 
> Added a test in the subsequent patch.
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>


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