Re: Review Request 43880: Added allocated metrics for total and allocated scalar resources.

2016-03-22 Thread Ben Mahler 

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


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  like we did with quota.



docs/monitoring.md (line 887)


s/of/or/

Did you do a self review? ;)



docs/monitoring.md (line 908)


s/of/or/



src/master/allocator/mesos/hierarchical.hpp (lines 290 - 294)


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)


s/resourceName/resource/

s/std:://



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


s/.get()./->/



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


s/resourceName/resource/

s/std:://



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


s/.get()./->/



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


two newlines here



src/master/allocator/mesos/metrics.hpp (lines 58 - 62)


s/kind//

How about `resources_total` and `resources_offered_or_allocated`?



src/master/allocator/mesos/metrics.cpp (lines 50 - 53)


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)


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



src/master/allocator/mesos/metrics.cpp (line 55)


newline here?



src/master/allocator/mesos/metrics.cpp (lines 56 - 61)


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)


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,
  ::_resources_total,
  resource));

Gauge offered_or_allocated(
"allocator/mesos/resources/" + resource + "/offered_or_allocated",
defer(allocator,
  
::_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)


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



src/tests/hierarchical_allocator_tests.cpp (lines 2437 - 2438)

Re: Review Request 43880: Added allocated metrics for total and allocated scalar resources.

2016-03-15 Thread Benjamin Bannier 

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

(Updated March 15, 2016, 3:51 p.m.)


Review request for mesos, Alexander Rukletsov and Ben Mahler.


Changes
---

Addressed offline comments from Ben Mahler.


Summary (updated)
-

Added allocated metrics for total and allocated scalar resources.


Bugs: MESOS-4720
https://issues.apache.org/jira/browse/MESOS-4720


Repository: mesos


Description (updated)
---

Added allocated metrics for total and allocated scalar resources.


Diffs (updated)
-

  docs/monitoring.md 827f7073204fcf8575ca980a5571c8be4f5e4110 
  src/master/allocator/mesos/hierarchical.hpp 
52b3a9bfbe73d24968f7ddb0672aee1e05636ab0 
  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