This is an automatically generated e-mail. To reply, visit:

src/master/allocator/mesos/hierarchical.hpp (line 478)

    Not very sure about the _Installs metrics_ since metrics are usually not 
installed but taken. The comment is lost outside the context, how about: 
_Installs gauges to obtain metrics of resources of kind `name`_ or something 
along those lines.

src/master/allocator/mesos/hierarchical.hpp (line 479)

    s/add/create/ when I see a method called add I generally assume I am 
passing what is going to be added (in this case, I though about passing the 
resource as a second parameter).

src/master/allocator/mesos/hierarchical.cpp (lines 413 - 418)

    This is really unreadable. I wasn't unsure of what it was happening here. I 
felt like an `std::initializer_list` was being created with two elements of 
complete different types.
    Then I realized that it was called implicitly the constructor of 
`process::metrics::Gauge`. So there are some fixes I feel these lines need:
    1. No implicit constructor calls, let's not make it hard for people reading 
the code so that they will need to jump back to see the declaration of 
    2. There aren't many constructors in this file, but the few there use 
parenthesis instead of brackets. Let's aim for consistency and readability 
instead of brace-initialization lets use parenthesis.
    3. I think creating the deferred object outside will improve readability.
    All in all, I would go for code that looks like:
    Deferred<Future<double>()> totalGenerator = 
      process::defer(self(), [this, name]() {
        Option<Value::Scalar> value =
        return value.isSome() ? value.get().value() ? 0.0;
            strings::join("/", "allocator/allocated", name),

src/master/allocator/mesos/hierarchical.cpp (lines 424 - 435)

    Same as above.

src/master/allocator/mesos/hierarchical.cpp (line 426)


- Alexander Rojas

On Feb. 28, 2016, 10:28 p.m., Benjamin Bannier wrote:
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43880/
> -----------------------------------------------------------
> (Updated Feb. 28, 2016, 10:28 p.m.)
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
> Bugs: MESOS-4720
>     https://issues.apache.org/jira/browse/MESOS-4720
> Repository: mesos
> Description
> -------
> Added allocator metrics for total and allocated scalar resources.
> Diffs
> -----
>   docs/monitoring.md 827f7073204fcf8575ca980a5571c8be4f5e4110 
>   src/master/allocator/mesos/hierarchical.hpp 
> 3043888630b066505410d3b32c5b3f813cc458c1 
>   src/master/allocator/mesos/hierarchical.cpp 
> 24fa50f62dec10ed43089297473cc386d6ba2f78 
>   src/tests/hierarchical_allocator_tests.cpp 
> 5f771f02db9bd098f3cd36730cd84bf2f5e87a33 
> Diff: https://reviews.apache.org/r/43880/diff/
> Testing
> -------
> make check (OS X)
> I confirmed that this does not lead to general performance regressions in the 
> allocator; this is partially expected since the added code only inserts 
> metrics in the allocator while the actual work is perform asynchronously. 
> These tests where performed with 
> `HierarchicalAllocator_BENCHMARK_Test.DeclineOffers` on an optimized build 
> under OS X using clang(trunk) as compiler.
> Thanks,
> Benjamin Bannier

Reply via email to