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