> On Jan. 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?

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!


- Alexander


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


On Dec. 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 Dec. 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
> 
>

Reply via email to