mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Joris Van Remoortere" <joris.van.remoort...@gmail.com>
Subject Re: Review Request 40332: Quota: Implemented recovery in hierarchical allocator.
Date Mon, 23 Nov 2015 17:54:13 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40332/#review107607
-----------------------------------------------------------



src/master/allocator/mesos/hierarchical.hpp (line 196)
<https://reviews.apache.org/r/40332/#comment166846>

    s/Methods/Functions/Helpers/
    Methods is more of a java-ism :-)



src/master/allocator/mesos/hierarchical.cpp (lines 177 - 178)
<https://reviews.apache.org/r/40332/#comment166833>

    This first invariant is something we push on the user of the allocator API (i.e. don't
call `addSlave()` until after `recover()` is called).
    The second is an internal invariant we maintain between `initialize()` and `recover()`.
    Can we clarify this with a comment, maybe even split them out in separate code blocks?
    
    I know this seems picky, but we try not to use `CHECK`s for external invariants (like
L177). We happen to do so currently in the allocator because it is so tightly integrated with
the master. Ideally we can remove these external ones once we refactor; however, we would
want to keep the internal one.
    Does this make sense?



src/master/allocator/mesos/hierarchical.cpp (line 184)
<https://reviews.apache.org/r/40332/#comment166834>

    new line.



src/master/allocator/mesos/hierarchical.cpp (line 187)
<https://reviews.apache.org/r/40332/#comment166835>

    s/after/after a/



src/master/allocator/mesos/hierarchical.cpp (lines 191 - 193)
<https://reviews.apache.org/r/40332/#comment166838>

    Is there a ticket for this? I think we'll want to do this for the MVP.
    Thoughts?



src/master/allocator/mesos/hierarchical.cpp (line 198)
<https://reviews.apache.org/r/40332/#comment166839>

    s/Record a/Record the/



src/master/allocator/mesos/hierarchical.cpp (lines 199 - 200)
<https://reviews.apache.org/r/40332/#comment166840>

    Let's drop the `this`, and rename the parameter with a `_`.



src/master/allocator/mesos/hierarchical.cpp (line 412)
<https://reviews.apache.org/r/40332/#comment166843>

    Should we `CHECK(!paused || expectedAgentCount.isSome());` above this?



src/master/allocator/mesos/hierarchical.cpp (lines 413 - 418)
<https://reviews.apache.org/r/40332/#comment166844>

    Rather than doing the math here  (which I believe we're missing corresponding entries
for in `removeSlave()`?) why not test whether `slaves.size() >= expectedAgentCount()` ?
    
    I think it is more clear, and less error prone.
    
    Let's log when this condition is met?



src/master/allocator/mesos/hierarchical.cpp (lines 988 - 997)
<https://reviews.apache.org/r/40332/#comment166836>

    Let's most definitely log this :-)
    It should happen rarely, so it's not a big deal to log.
    Considering the conversations we had with other developers, this is definitely something
they will want to know when debugging ;-)



src/master/allocator/mesos/hierarchical.cpp (lines 994 - 997)
<https://reviews.apache.org/r/40332/#comment166845>

    Does it make sense to `CHECK(paused)` here?
    If not, why not? I think making these semantics as clear as possible is crucial for such
a high impact change.



src/master/allocator/mesos/hierarchical.cpp (lines 1009 - 1010)
<https://reviews.apache.org/r/40332/#comment166837>

    Let's log that we are skipping allocation due to a paused allocator.
    Same for the keyed version of this function below.


- Joris Van Remoortere


On Nov. 23, 2015, 4:11 p.m., Alexander Rukletsov wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40332/
> -----------------------------------------------------------
> 
> (Updated Nov. 23, 2015, 4:11 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joerg Schad, Joris Van Remoortere, Joseph Wu,
and Qian Zhang.
> 
> 
> Bugs: MESOS-3981
>     https://issues.apache.org/jira/browse/MESOS-3981
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> See summary.
> 
> 
> Diffs
> -----
> 
>   src/master/allocator/mesos/hierarchical.hpp c65fe35198b846da2dc959dd467a21ff6edd30a9

>   src/master/allocator/mesos/hierarchical.cpp 2765526047767cbd19d13c11ecfa6e90c505b3a7

> 
> Diff: https://reviews.apache.org/r/40332/diff/
> 
> 
> Testing
> -------
> 
> make check (Mac OS X 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>


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