mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jiang Yan Xu <...@jxu.me>
Subject Re: Review Request 47259: CHECK if DRFSorter::add() would introduce a duplicate.
Date Thu, 26 May 2016 00:13:32 GMT


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


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