mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alexander Rukletsov" <ruklet...@gmail.com>
Subject Re: Review Request 41769: Made allocator traverse all roles for quota allocation.
Date Wed, 13 Jan 2016 12:32:48 GMT


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


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