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 70276: Included RP resources / static reservations in quota capacity check.
Date Mon, 25 Mar 2019 23:56:35 GMT


> On March 24, 2019, 5:06 p.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.
> 
> Benjamin Mahler wrote:
>     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?

// Returns an error if the total quota guarantees overcommits
// the cluster. This is not a quota satisfiability check: it's
// possible that quota is unsatisfiable even if the quota
// does not overcommit the cluster. Specifically, we verify that
// the following inequality holds:
//
//    total cluster capacity >= total quota + quota request
//
// Note, total cluster capacity accounts resources of all the
// registered agents, including resources from resource providers
// as well as reservations (both static and dynamic ones).


> On March 24, 2019, 5:06 p.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.
> 
> Benjamin Mahler wrote:
>     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).

> behavior + refactoring mixing
I was referring to the RP/static reservation (looks like the revocable is not a behavior change
given your following comment) change and the quantities refactor. Given the small size of
the patch, I am OK with them mixed.

For the performance argument, not sure how the worst performance could be worse, given that
the subtraction and addition time should be comparable. And emptiness check is pretty quick.
But anyway, given the error messages update in the later patch where total cluster capacity
is required. Let's drop this as well.


> On March 24, 2019, 5:06 p.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?
> 
> Benjamin Mahler wrote:
>     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)

OK, that makes sense. Let me drop this.


- Meng


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


On March 22, 2019, 8:54 a.m., Benjamin Mahler wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70276/
> -----------------------------------------------------------
> 
> (Updated March 22, 2019, 8:54 a.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