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 43880: Added allocated metrics for total and allocated scalar resources.
Date Wed, 23 Mar 2016 00:56:06 GMT

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


Fix it, then Ship it!




Thanks for the patience Benjamin! I'll make some adjustments based on the comments below,
and get this committed shortly.


docs/monitoring.md (lines 869 - 917)
<https://reviews.apache.org/r/43880/#comment187560>

    Let's say a user makes a custom resource named "quota", then it mixes with the "quota"
directory we created earlier. The suggestion from the last review was to add a "resources/"
prefix, sound good?
    
    Once we follow up on ensuring that all resources are represented (not just cpus, mem,
disk), it would also be great to change this to just be parameterized on <resource>
like we did with quota.



docs/monitoring.md (line 887)
<https://reviews.apache.org/r/43880/#comment187607>

    s/of/or/
    
    Did you do a self review? ;)



docs/monitoring.md (line 908)
<https://reviews.apache.org/r/43880/#comment187606>

    s/of/or/



src/master/allocator/mesos/hierarchical.hpp (lines 290 - 294)
<https://reviews.apache.org/r/43880/#comment187608>

    Let's perhaps include the word resources when naming these, and omit the comments (since
we comment the metric insted of the function in the existing code).
    
    Let's do a s/resourceName/resource/ to be consistent with your last patch.
    
    How about `s/_total/_resources_total/` and `s/_allocated/_resources_allocated/` here and
on the gauge variable names to be a bit more clear? It's also consistent with the master's
approach to naming these gauges (note that this also fits well with naming the metric with
a "resources/" prefix).



src/master/allocator/mesos/hierarchical.cpp (line 1691)
<https://reviews.apache.org/r/43880/#comment187622>

    s/resourceName/resource/
    
    s/std:://



src/master/allocator/mesos/hierarchical.cpp (line 1700)
<https://reviews.apache.org/r/43880/#comment187618>

    s/.get()./->/



src/master/allocator/mesos/hierarchical.cpp (line 1708)
<https://reviews.apache.org/r/43880/#comment187621>

    s/resourceName/resource/
    
    s/std:://



src/master/allocator/mesos/hierarchical.cpp (line 1713)
<https://reviews.apache.org/r/43880/#comment187617>

    s/.get()./->/



src/master/allocator/mesos/hierarchical.cpp (line 1715)
<https://reviews.apache.org/r/43880/#comment187624>

    two newlines here



src/master/allocator/mesos/metrics.hpp (lines 58 - 62)
<https://reviews.apache.org/r/43880/#comment187616>

    s/kind//
    
    How about `resources_total` and `resources_offered_or_allocated`?



src/master/allocator/mesos/metrics.cpp (lines 50 - 53)
<https://reviews.apache.org/r/43880/#comment187610>

    Let's also pull in dominic's TODO about dynamically adding gauges:
    
    ```
    // Create resource gauges.
    //
    // TODO(bbannier) Add support for more than just scalar resources.
    // TODO(bbannier) Simplify this once MESOS-3214 is fixed.
    // TODO(dhamon): Set these up dynamically when adding a slave based on the
    // resources the slave exposes.
    ```



src/master/allocator/mesos/metrics.cpp (lines 54 - 55)
<https://reviews.apache.org/r/43880/#comment187612>

    s/resourceKinds/resources/ (since you called the singular loop variable 'resource' already..
:))



src/master/allocator/mesos/metrics.cpp (line 55)
<https://reviews.apache.org/r/43880/#comment187613>

    newline here?



src/master/allocator/mesos/metrics.cpp (lines 56 - 61)
<https://reviews.apache.org/r/43880/#comment187615>

    We don't need a map right now, we can just use a vector to be consistent with the master's
metrics. We can switch to a map when we start dynamically adding gauges.



src/master/allocator/mesos/metrics.cpp (lines 56 - 74)
<https://reviews.apache.org/r/43880/#comment187620>

    How about the following structure, which is similar to the master's metrics and seems
a bit cleaner?
    
    ```
      foreach (const string& resource, resources) {
        Gauge total(
            "allocator/mesos/resources/" + resource + "/total",
            defer(allocator,
                  &HierarchicalAllocatorProcess::_resources_total,
                  resource));
    
        Gauge offered_or_allocated(
            "allocator/mesos/resources/" + resource + "/offered_or_allocated",
            defer(allocator,
                  &HierarchicalAllocatorProcess::_resources_offered_or_allocated,
                  resource));
    
        resources_total.push_back(total);
        resources_offered_or_allocated.push_back(offered_or_allocated);
    
        process::metrics::add(total);
        process::metrics::add(offered_or_allocated);
      }
    ```



src/master/allocator/mesos/metrics.cpp (line 106)
<https://reviews.apache.org/r/43880/#comment187623>

    Whoops, I missed these earlier, no need for std:: qualifiers here and below.



src/tests/hierarchical_allocator_tests.cpp (lines 2437 - 2438)
<https://reviews.apache.org/r/43880/#comment187626>

    Try to wrap to reduce jaggedness:
    
    ```
    // This test checks that total and allocator resources
    // are correctly reflected in the metrics endpoint.
    ```



src/tests/hierarchical_allocator_tests.cpp (lines 2437 - 2486)
<https://reviews.apache.org/r/43880/#comment187628>

    Hm.. seems contains is more readable? Especially once the "resources/" prefix gets added
because a lot of lines need to get wrapped:
    
    ```
    // This test checks that total and allocator resources
    // are correctly reflected in the metrics endpoint.
    TEST_F(HierarchicalAllocatorTest, ResourceMetrics)
    {
      // Pausing the clock is not necessary, but ensures that the test
      // doesn't rely on the batch allocation in the allocator, which
      // would slow down the test.
      Clock::pause();
    
      initialize();
    
      SlaveInfo agent = createSlaveInfo("cpus:2;mem:1024;disk:0");
      allocator->addSlave(agent.id(), agent, None(), agent.resources(), {});
      Clock::settle();
    
      JSON::Object expected;
    
      // No frameworks are registered yet, so nothing is allocated.
      expected.values = {
          {"allocator/mesos/resources/cpus/total",   2},
          {"allocator/mesos/resources/mem/total", 1024},
          {"allocator/mesos/resources/disk/total",   0},
          {"allocator/mesos/resources/cpus/offered_or_allocated", 0},
          {"allocator/mesos/resources/mem/offered_or_allocated",  0},
          {"allocator/mesos/resources/disk/offered_or_allocated", 0},
      };
    
      JSON::Value metrics = Metrics();
    
      EXPECT_TRUE(metrics.contains(expected));
    
      FrameworkInfo framework = createFrameworkInfo("role1");
      allocator->addFramework(framework.id(), framework, {});
      Clock::settle();
    
      // All of the resources should be offered.
      expected.values = {
          {"allocator/mesos/resources/cpus/total",   2},
          {"allocator/mesos/resources/mem/total", 1024},
          {"allocator/mesos/resources/disk/total",   0},
          {"allocator/mesos/resources/cpus/offered_or_allocated",   2},
          {"allocator/mesos/resources/mem/offered_or_allocated", 1024},
          {"allocator/mesos/resources/disk/offered_or_allocated",   0},
      };
    
      metrics = Metrics();
    
      EXPECT_TRUE(metrics.contains(expected));
    
      allocator->removeSlave(agent.id());
      Clock::settle();
    
      // No frameworks are registered yet, so nothing is allocated.
      expected.values = {
          {"allocator/mesos/resources/cpus/total", 0},
          {"allocator/mesos/resources/mem/total",  0},
          {"allocator/mesos/resources/disk/total", 0},
          {"allocator/mesos/resources/cpus/offered_or_allocated", 0},
          {"allocator/mesos/resources/mem/offered_or_allocated",  0},
          {"allocator/mesos/resources/disk/offered_or_allocated", 0},
      };
    
      metrics = Metrics();
    
      EXPECT_TRUE(metrics.contains(expected));
    }
    ```


- Ben Mahler


On March 21, 2016, 11:08 a.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43880/
> -----------------------------------------------------------
> 
> (Updated March 21, 2016, 11:08 a.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 allocated metrics for total and allocated scalar resources.
> 
> 
> Diffs
> -----
> 
>   docs/monitoring.md 827f7073204fcf8575ca980a5571c8be4f5e4110 
>   src/master/allocator/mesos/hierarchical.hpp 0c622b8569bc79ae4e365a57e463ca5349356713

>   src/master/allocator/mesos/hierarchical.cpp 70291075c00a9a557529c2562dedcfc6c6c3ec32

>   src/master/allocator/mesos/metrics.hpp d04e9a11c77b6fb2d522608e3085f81f8a6657ca 
>   src/master/allocator/mesos/metrics.cpp 46dd7f7b3d8d49c59b42e789fe5dcd49f1693ace 
>   src/tests/hierarchical_allocator_tests.cpp 459e02576f6d05abbbcc83ae5cabac5c66e93f05

> 
> 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
> 
>


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