mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Guangya Liu" <gyliu...@gmail.com>
Subject Re: Review Request 41769: Made allocator traverse all roles for quota allocation.
Date Wed, 13 Jan 2016 15:16:06 GMT


> On 一月 13, 2016, 11:41 a.m., Alexander Rukletsov wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 1164-1166
> > <https://reviews.apache.org/r/41769/diff/2/?file=1180221#file1180221line1164>
> >
> >     This looks like a bug to me. I can't remember why we have `break` here in the
first place. Could you please elaborate why do you think `break` is fine here?
> 
> Guangya Liu wrote:
>     It is a bit tricky here so I add some comments here even though I prefer we use `continue`
here.
>     
>     The reason that it is OK to use `break` is because all of the quota roles are sorted
based on starvation, if the roles in quota role sorter is satisfied, we can assume that all
roles behind current satisfied role is also satisfied, that's why `break` here. Comments?
> 
> Alexander Rukletsov wrote:
>     I'm not sure it's right. Roles are sorted according to their allocation, hence a
role with quota 5 and allocation 5 will come before role with quota 50 and allocation 10.
I believe `continue` should be used here. Also imagine a custom sorter is used, which does
not guarantee any ordering.
>     
>     Anyway, I've checked all the tests we have and it seems there is no test where we
set quotas for multiple roles. Do you want to add such test or expand an existing one? Also
we may want to add some tests around implicit roles and cover the other issue you fix in this
test. There is a ticket for this already: MESOS-4292. We have to figure out what exactly we
want to test and sync with NeilC.
>     
>     I'd be happy to help out and review, this looks like a bug! 
>     
>     Could you please also keep Joris and Qian Zhang in the loop? Thanks!

Yes, you are right, I think that here still should use `continue` here. Will try to add a
new test cases for this.


- Guangya


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


On 十二月 31, 2015, 11:34 p.m., Guangya Liu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41769/
> -----------------------------------------------------------
> 
> (Updated 十二月 31, 2015, 11:34 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Klaus Ma, and Neil Conway.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Made allocator traverse all roles for quota allocation.
> 
> 
> Diffs
> -----
> 
>   src/master/allocator/mesos/hierarchical.cpp 7f900c4e024485704d79e57ae22407557598fe6c

> 
> Diff: https://reviews.apache.org/r/41769/diff/
> 
> 
> Testing
> -------
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>


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