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 17:15:40 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?
> 
> 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
> 
>


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