Re: Review Request 44853: Added benchmark test for the allocator metrics endpoint.

[Ben Mahler]

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


Fix it, then Ship it!





src/tests/hierarchical_allocator_tests.cpp (line 2988)


2 spaces here



src/tests/hierarchical_allocator_tests.cpp (lines 3000 - 3017)


size_t above but unsigned here?

Also, why the vectors here?

```
  for (size_t i = 0; i < frameworkCount; i++) {
string role = stringify(i);
allocator->setQuota(role, createQuota(role, "cpus:1;mem:512;disk:256"));
  }

  for (size_t i = 0; i < frameworkCount; i++) {
FrameworkInfo framework = createFrameworkInfo(stringify(i)));
allocator->addFramework(framework.id(), framework, {});
  }

  for (size_t i = 0; i < slaveCount; i++) {
SlaveInfo slave = createSlaveInfo("cpus:16;mem:2048;disk:1024");
allocator->addSlave(slaveid(), slave, None(), slave.resources(), {});
  }
```



src/tests/hierarchical_allocator_tests.cpp (lines 3022 - 3032)


Why the scope here? Looks like it was added because some of the new 
benchmarks in this file are using a scope for the stopwatch? I don't think it's 
bad, but it's a little tricky to follow the reasoning for where to use a scope 
here. I suppose it calls out the timing code, but I'd opt to just avoid the 
extra scoping for now.



src/tests/hierarchical_allocator_tests.cpp (lines 3023 - 3027)


Perhaps this would be clearer?

```
  Stopwatch watch;

  // TODO(bmahler): Avoid timing the JSON parsing here.
  // Ideally we also avoid timing the HTTP layer.
  watch.start();
  JSON::Object metrics = Metrics();
  watch.stop();
```



src/tests/hierarchical_allocator_tests.cpp (lines 3029 - 3031)


How about:

```
  cout << "/metrics/snapshot took " << watch.elapsed()
   << " for " << slaveCount << " slaves"
   << " and " << frameworkCount << " frameworks" << endl;
```


- Ben Mahler


On March 30, 2016, 1:22 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44853/
> ---
> 
> (Updated March 30, 2016, 1:22 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added benchmark test for the allocator metrics endpoint.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 459e02576f6d05abbbcc83ae5cabac5c66e93f05 
> 
> Diff: https://reviews.apache.org/r/44853/diff/
> 
> 
> Testing
> ---
> 
> The benchmark uses the same parametrized setup as other 
> `HierarchicalAllocator_BENCHMARK_Tests` which already elsewhere take 
> considerable time. The reason for covering the same parameter space here was 
> the assumption that that parameter space does capture the relevant scenarios.
> 
> The benchmark shows that the time needed to obtain the metrics has a linear 
> relationship with the number of registered frameworks, roughly independent of 
> the number of slaves. With my setup, the time per framework was well below 1 
> ms after already a few frameworks.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 44853: Added benchmark test for the allocator metrics endpoint.

[Benjamin Bannier]

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

(Updated March 17, 2016, 3:49 p.m.)


Review request for mesos, Alexander Rukletsov and Ben Mahler.


Changes
---

Used stringify instead of string::to_string.


Repository: mesos


Description
---

Added benchmark test for the allocator metrics endpoint.


Diffs (updated)
-

  src/tests/hierarchical_allocator_tests.cpp 
459e02576f6d05abbbcc83ae5cabac5c66e93f05 

Diff: https://reviews.apache.org/r/44853/diff/


Testing
---

The benchmark uses the same parametrized setup as other 
`HierarchicalAllocator_BENCHMARK_Tests` which already elsewhere take 
considerable time. The reason for covering the same parameter space here was 
the assumption that that parameter space does capture the relevant scenarios.

The benchmark shows that the time needed to obtain the metrics has a linear 
relationship with the number of registered frameworks, roughly independent of 
the number of slaves. With my setup, the time per framework was well below 1 ms 
after already a few frameworks.


Thanks,

Benjamin Bannier

Re: Review Request 44853: Added benchmark test for the allocator metrics endpoint.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [44853]

Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' 
COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh

- Mesos ReviewBot


On March 15, 2016, 2:52 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44853/
> ---
> 
> (Updated March 15, 2016, 2:52 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added benchmark test for the allocator metrics endpoint.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 459e02576f6d05abbbcc83ae5cabac5c66e93f05 
> 
> Diff: https://reviews.apache.org/r/44853/diff/
> 
> 
> Testing
> ---
> 
> The benchmark uses the same parametrized setup as other 
> `HierarchicalAllocator_BENCHMARK_Tests` which already elsewhere take 
> considerable time. The reason for covering the same parameter space here was 
> the assumption that that parameter space does capture the relevant scenarios.
> 
> The benchmark shows that the time needed to obtain the metrics has a linear 
> relationship with the number of registered frameworks, roughly independent of 
> the number of slaves. With my setup, the time per framework was well below 1 
> ms after already a few frameworks.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Review Request 44853: Added benchmark test for the allocator metrics endpoint.

[Benjamin Bannier]

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

Review request for mesos, Alexander Rukletsov and Ben Mahler.


Summary (updated)
-

Added benchmark test for the allocator metrics endpoint.


Repository: mesos


Description (updated)
---

Added benchmark test for the allocator metrics endpoint.


Diffs (updated)
-

  src/tests/hierarchical_allocator_tests.cpp 
459e02576f6d05abbbcc83ae5cabac5c66e93f05 

Diff: https://reviews.apache.org/r/44853/diff/


Testing (updated)
---

The benchmark uses the same parametrized setup as other 
`HierarchicalAllocator_BENCHMARK_Tests` which already elsewhere take 
considerable time. The reason for covering the same parameter space here was 
the assumption that that parameter space does capture the relevant scenarios.

The benchmark shows that the time needed to obtain the metrics has a linear 
relationship with the number of registered frameworks, roughly independent of 
the number of slaves. With my setup, the time per framework was well below 1 ms 
after already a few frameworks.


Thanks,

Benjamin Bannier