> On May 25, 2016, 1:28 p.m., Joris Van Remoortere wrote: > > src/master/allocator/sorter/drf/sorter.cpp, line 50 > > <https://reviews.apache.org/r/47259/diff/1/?file=1379947#file1379947line50> > > > > Why can this be a `CHECK` if the previous review can't? > > What is the standard we're trying to set for invariants? > > Jiang Yan Xu wrote: > We have been using CHECKs and if guards inconsistently in the sorter. > > Normally we shouldn't use CHECKs on public method arguments (i.e., they > are not invariants) but it's widely used in sorter due to the tight coupling > between the allocator and the sorter and it's either simpler for the > allocator to not have to handle an error/none/false result or sometimes it's > impossible to handle it. I filed MESOS-5280 for the inconsistency but now I > feel it's perhaps not worth making all the methods return error/none/false > instead of doing CHECKs, it could be discussed there though. > > So if we don't replace CHECKs with error/none/false returns, should we > always CHECK and crash the problem? I feel that will be too much. > > The difference between this patch and the previous is (as I commented on > MESOS-5279): > - It's acceptable to do nothing when the **exact** thing it is asked to > do is already done (this doesn't get us into a bad interanl state). (The case > in /r/47258/) > - It's not acceptable to silently refuse to do something not because it's > already done but it's impossible to do it because it gets us into a bad > internal state (duplicate entries). (The case in here /r/47259/, as the > caller could first call `sorter->add("fw1", 1)` and then `sorter->add("fw1", > 2)` which would be a **different** thing) > > P.S. We should always CHECK internal invariants in the sorter for sure.
As a "standard" for now I feel that we should 1. CHECK internal invariants 2. CHECK invalid arguments that would get the sorter into a bad state and it doesn't make sense to make it a no-op. (e.g., [here](https://github.com/apache/mesos/blob/6ce476461f0fedfb4ed4e40c15f25bb79a39b0f3/src/master/allocator/sorter/drf/sorter.cpp#L147) update() runs ` CHECK(contains(name));`) 3. Use if guards when it make sense the method can be a noop (e.g., [here](https://github.com/apache/mesos/blob/6ce476461f0fedfb4ed4e40c15f25bb79a39b0f3/src/master/allocator/sorter/drf/sorter.cpp#L95) deactive() doesn't CHECK if the name is "active") 4. We should never *assume* the allocator will call sorter methods in the expected way. (e.g., [here](https://github.com/apache/mesos/blob/6ce476461f0fedfb4ed4e40c15f25bb79a39b0f3/src/master/allocator/sorter/drf/sorter.cpp#L242) the method has no safegards at all. For 2. I don't know the state of the rework but if there is an overhaul ideally the sorter should still return error/none/false for invalid arguments and the allocator can have CHECKs on the return value because it may be the allocator's **internal invariants**. I can sign up for this (will update MESOS-5280 to reflect the dicussion here) but I don't think MESOS-5279 depends on it. Thoughts? - Jiang Yan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/47259/#review134829 ----------------------------------------------------------- On May 19, 2016, 11:28 a.m., Jiang Yan Xu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/47259/ > ----------------------------------------------------------- > > (Updated May 19, 2016, 11:28 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 > ------- > > CHECK if DRFSorter::add() would introduce a duplicate. > > > Diffs > ----- > > src/master/allocator/sorter/drf/sorter.cpp > 4306973277b9d32356eed31ceabac09fb2a03e6c > > Diff: https://reviews.apache.org/r/47259/diff/ > > > Testing > ------- > > make check. > > > Thanks, > > Jiang Yan Xu > >
