> On May 25, 2016, 1:24 p.m., Joris Van Remoortere wrote: > > src/master/allocator/sorter/drf/sorter.cpp, lines 90-91 > > <https://reviews.apache.org/r/47258/diff/1/?file=1379945#file1379945line90> > > > > Is this a guard as opposed to a `CHECK` because someone is using this > > API correctly and we are mitigating the performance impact of that? > > > > We could argue that it shouldn't be a `CHECK` because it's not an > > internal variant; however, I would like to know if there's a higher level > > bug we should be fixing :-) > > Jiang Yan Xu wrote: > I don't think it should be a CHECK here but it should be a CHECK in > /r/47259/ as I commented on /r/47259/ for CHECK vs. if guard. > > The motivation for this patch is from debugging /r/43666/. > > > We could argue that it shouldn't be a CHECK because it's not an > internal variant; > > I think this is exactly right. The "high-level bug" is probably that due > to the tight coupling between the allocator and the sorter we are forced to > CHECK public method arguments as well. I think we should miminize this.
Plus we can look at the [deactive](https://github.com/apache/mesos/blob/6ce476461f0fedfb4ed4e40c15f25bb79a39b0f3/src/master/allocator/sorter/drf/sorter.cpp#L95) as an example: if it's already not "active", deactivate is a no-op. - Jiang Yan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/47258/#review134826 ----------------------------------------------------------- On May 19, 2016, 11:27 a.m., Jiang Yan Xu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/47258/ > ----------------------------------------------------------- > > (Updated May 19, 2016, 11:27 a.m.) > > > Review request for mesos, Dario Rexin and Joris Van Remoortere. > > > Bugs: MESOS-5279 > https://issues.apache.org/jira/browse/MESOS-5279 > > > Repository: mesos > > > Description > ------- > > Make sure 'activate' is a no-op if the client is already active. > > > Diffs > ----- > > src/master/allocator/sorter/drf/sorter.cpp > 4306973277b9d32356eed31ceabac09fb2a03e6c > src/master/allocator/sorter/sorter.hpp > 9e04adf54f2d80541a95f0a9a49b329dc9e8f5e3 > > Diff: https://reviews.apache.org/r/47258/diff/ > > > Testing > ------- > > make check. > > > Thanks, > > Jiang Yan Xu > >
