mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Mahler <bmah...@apache.org>
Subject Re: Review Request 66870: Added per-framework metrics for suppressed roles.
Date Wed, 25 Jul 2018 23:58:35 GMT

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


Fix it, then Ship it!




Overall looks good, but there's a significant existing issue below that I want to make sure
we follow up on.


src/master/allocator/mesos/hierarchical.cpp
Line 287 (original), 288 (patched)
<https://reviews.apache.org/r/66870/#comment289466>

    It jumps out to the reader as a potential bug that the activate case doesn't have a `reviveRole`
call to make it symmetric. Shall we just add one? Is this activate call a no-op?



src/master/allocator/mesos/hierarchical.cpp
Lines 734-758 (original), 747-772 (patched)
<https://reviews.apache.org/r/66870/#comment289467>

    I'm not familiar with this, but it reads as wrong. Why does clearing the agent filters
modify the framework's suppressed roles? That breaks the API contract in which frameworks
are in control of their suppression state.
    
    I don't think we should tweak their suppression state based on events, we should instead
expose those events to schedulers and let them decide.
    
    Can you follow up with a ticket / fix for this or ask (benno i think?) to follow up?



src/master/allocator/mesos/metrics.hpp
Lines 104-105 (patched)
<https://reviews.apache.org/r/66870/#comment289465>

    This is quickly going to be stale when we add the update call that kapil is working on,
maybe just say that frameworks can update their roles without saying when?



src/master/allocator/mesos/metrics.hpp
Lines 111 (patched)
<https://reviews.apache.org/r/66870/#comment289459>

    This wasn't obvious to me at first glance, how about:
    
    ```
    // Suppresion state metric (boolean 0 or 1) for each role.
    ```



src/master/allocator/mesos/metrics.cpp
Lines 220-224 (patched)
<https://reviews.apache.org/r/66870/#comment289463>

    It's fine for now since we don't have to deal with frameworks with large numbers of roles
yet, but `getRoles` copies the roles out from `frameworkInfo` into a set, and we only need
to iterate through them here so it's a bit wasteful. A TODO?



src/master/allocator/mesos/metrics.cpp
Lines 213-215 (original), 238-271 (patched)
<https://reviews.apache.org/r/66870/#comment289460>

    These are not known hot paths (although `addRole()` might be in the future with multi-role
frameworks with large numbers of roles), but consider removing the redundant lookups in these
if you can keep the code readable.



src/master/allocator/mesos/metrics.cpp
Lines 242 (patched)
<https://reviews.apache.org/r/66870/#comment289461>

    Do you need the `.`? It should convert implicitly to double?
    
    If you do need it, can we do `0.0` and `1.0`? That seems a little more readable?



src/master/allocator/mesos/metrics.cpp
Lines 250 (patched)
<https://reviews.apache.org/r/66870/#comment289462>

    Ditto here.


- Benjamin Mahler


On July 24, 2018, 3:40 p.m., Greg Mann wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66870/
> -----------------------------------------------------------
> 
> (Updated July 24, 2018, 3:40 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Gastón Kleiman, Gilbert Song, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added per-framework metrics for suppressed roles.
> 
> 
> Diffs
> -----
> 
>   src/master/allocator/mesos/hierarchical.cpp 707dd6bd0f255a64d759ce87cbf75be57d86b392

>   src/master/allocator/mesos/metrics.hpp 6d386225c301d5ab44f3cc0ecdd1478fb5162e5b 
>   src/master/allocator/mesos/metrics.cpp 82990b2dc0b827a43a392d898667eaf58c77ea36 
> 
> 
> Diff: https://reviews.apache.org/r/66870/diff/6/
> 
> 
> Testing
> -------
> 
> Testing details can be found at the end of this chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>


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