mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ben Mahler <benjamin.mah...@gmail.com>
Subject Re: Review Request 44260: Moved metrics of the hierarchical allocator to its own file.
Date Sat, 05 Mar 2016 04:46:04 GMT

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


Fix it, then Ship it!




I like the idea of making this more consistent with the metrics within `master/` and `slave/`,
thanks!

A couple of minor things here but otherwise looks great!


src/master/allocator/mesos/hierarchical.hpp (line 284)
<https://reviews.apache.org/r/44260/#comment184092>

    Hm.. I don't know that the comment here adds any clarity for the reader. At least for
me, it's not clear what it means for a dispatch event to be "waiting", this is just the number
of dispatches in the event queue. "wait" seems to imply some form of blocking.
    
    It seems pretty clear to me from the way the code is written here and the metric name,
no?
    
    Also, keep in mind that this very name is used in a number of places, so the documentation
should ideally be consistent.



src/master/allocator/mesos/metrics.hpp (lines 1 - 47)
<https://reviews.apache.org/r/44260/#comment184103>

    The directory layout / file naming here is a little odd, because this is coupled to the
hierarchical allocator, but the following layout doesn't express this:
    
    `allocator/mesos/allocator.hpp`
    `allocator/mesos/metrics.hpp`
    `allocator/mesos/hierarchical.hpp`
    
    Ideally the hierarchical allocator and metrics are more clearly coupled, since there is
the allocator wrapper also contained here. For now it's ok since we can move things around
here (not part of the public interfaces).



src/master/allocator/mesos/metrics.cpp (lines 17 - 21)
<https://reviews.apache.org/r/44260/#comment184104>

    Any reason for the unusual ordering?


- Ben Mahler


On March 3, 2016, 4:17 p.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44260/
> -----------------------------------------------------------
> 
> (Updated March 3, 2016, 4:17 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
> 
> 
> Bugs: MESOS-4718
>     https://issues.apache.org/jira/browse/MESOS-4718
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Moved metrics of the hierarchical allocator to its own file.
> 
> 
> Diffs
> -----
> 
>   src/CMakeLists.txt 0eabfad66f43adff12d08dbf34ca3db1b801c49e 
>   src/Makefile.am b30cc25f61856d6417437547baaa0bb338a30d63 
>   src/master/allocator/mesos/hierarchical.hpp 3043888630b066505410d3b32c5b3f813cc458c1

>   src/master/allocator/mesos/metrics.hpp PRE-CREATION 
>   src/master/allocator/mesos/metrics.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44260/diff/
> 
> 
> Testing
> -------
> 
> `make distcheck` on OS X.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>


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