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