Re: Review Request 43882: Added allocation metrics for allocation time.

2016-03-23 Thread Ben Mahler 

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



Thanks! I'll make the adjustments from the comments below, please take a look 
at the commit diff.


docs/monitoring.md (lines 906 - 910)


Hm.. how about allocation_run to be more consistent with our existing timer 
names?



docs/monitoring.md (line 909)


s/Gauge/Timer/



src/master/allocator/mesos/metrics.hpp (lines 87 - 88)


Could we put this adjacent to the related `allocation_runs` metric?



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


Since we expect many allocations, we should pass a window here (see the 
registrar's `state_store` metric where we used a window of `Days(1)`). Passing 
a window allows us to obtain percentiles which is critical for events that 
occur on a regular basis where the monitoring system may not be able to catch 
every timing.

Here we could use 1 hour, since it's more freqent than the registry 
operations.



src/tests/hierarchical_allocator_tests.cpp (lines 2720 - 2747)


I ended up taking a stab at re-writing this given all the other comments:

```
// This test checks that the allocation run timer
// metrics are reported in the metrics endpoint.
TEST_F(HierarchicalAllocatorTest, AllocationRunTimerMetrics)
{
  Clock::pause();

  initialize();

  Clock::settle();

  // These time series statistics will be generated
  // once at least 2 allocation runs occur.
  auto statistics = {
"allocator/mesos/allocation_run_ms/count",
"allocator/mesos/allocation_run_ms/min",
"allocator/mesos/allocation_run_ms/max",
"allocator/mesos/allocation_run_ms/p50",
"allocator/mesos/allocation_run_ms/p95",
"allocator/mesos/allocation_run_ms/p99",
"allocator/mesos/allocation_run_ms/p999",
"allocator/mesos/allocation_run_ms/p",
  };

  JSON::Object metrics = Metrics();
  map values = metrics.values;

  EXPECT_EQ(1u, values.count("allocator/mesos/allocation_run_ms"));

  // No allocation timing statistics should appear.
  foreach (const string& statistic, statistics) {
EXPECT_EQ(0u, values.count(statistic))
  << "Expected " << statistic << " to be absent";
  }

  // Allow the allocation timer to measure time.
  Clock::resume();

  // Trigger at least two allocations to generate
  // the window statistics.
  SlaveInfo agent = createSlaveInfo("cpus:2;mem:1024;disk:0");
  allocator->addSlave(agent.id(), agent, None(), agent.resources(), {});
  AWAIT_READY(allocations.get());

  FrameworkInfo framework = createFrameworkInfo("role1");
  allocator->addFramework(framework.id(), framework, {});
  AWAIT_READY(allocations.get());

  metrics = Metrics();
  values = metrics.values;

  // A non-zero measurement should be present.
  JSON::Value value = metrics.values["allocator/mesos/allocation_time_ms"];
  ASSERT_TRUE(value.is());
  JSON::Number timing = value.as();
  ASSERT_EQ(JSON::Number::FLOATING, timing.type);
  EXPECT_GT(timing.as(), 0.0);

  // The statistics should be generated.
  foreach (const string& statistic, statistics) {
EXPECT_EQ(1u, values.count(statistic))
  << "Expected " << statistic << " to be absent";
  }
}
```



src/tests/hierarchical_allocator_tests.cpp (lines 2722 - 2723)


Hm.. the clock should be paused initially if you're assuming the metric is 
absent, otherwise the allocation interval could fire and insert the metric 
before you obtained the metrics.



src/tests/hierarchical_allocator_tests.cpp (line 2724)


brace on the next line



src/tests/hierarchical_allocator_tests.cpp (line 2733)


Hm.. it's unfortunate that there are a bunch of tests using "agent" as the 
variable name, seems confusing to the reader. I would have rather we did a 
consistent change across the tests, but not your fault, we can stick with agent 
here since this file seems to use it in many places.



src/tests/hierarchical_allocator_tests.cpp (lines 2736 - 2737)


Try to wrap to reduce jagedness:

```
  // Wait until at least one allocation run occurs
  // so the allocation time could be measured.
  Future allocation =

Re: Review Request 43882: Added allocation metrics for allocation time.

2016-03-21 Thread Benjamin Bannier 

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

(Updated March 21, 2016, 12:08 p.m.)


Review request for mesos, Alexander Rukletsov and Ben Mahler.


Changes
---

Review comments from @bmahler.


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


Repository: mesos


Description
---

Added allocation metrics for allocation time.


Diffs (updated)
-

  docs/monitoring.md 827f7073204fcf8575ca980a5571c8be4f5e4110 
  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/43882/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

Re: Review Request 43882: Added allocation metrics for allocation time.

2016-03-15 Thread Benjamin Bannier 

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

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


Review request for mesos, Alexander Rukletsov and Ben Mahler.


Changes
---

Addressed offline comments from Ben Mahler.


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


Repository: mesos


Description (updated)
---

Added allocation metrics for allocation time.


Diffs (updated)
-

  docs/monitoring.md 827f7073204fcf8575ca980a5571c8be4f5e4110 
  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/43882/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

Re: Review Request 43882: Added allocation metrics for allocation time.

2016-03-04 Thread Alexander Rukletsov 

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


Fix it, then Ship it!





src/tests/hierarchical_allocator_tests.cpp (lines 2568 - 2569)


Let's pull it into the test preambular comment.



src/tests/hierarchical_allocator_tests.cpp (lines 2578 - 2579)


s/agent1/agent



src/tests/hierarchical_allocator_tests.cpp (line 2581)


s/least/at least


- Alexander Rukletsov


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/43882/
> ---
> 
> (Updated March 3, 2016, 4:17 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
> 
> 
> Bugs: MESOS-4721
> https://issues.apache.org/jira/browse/MESOS-4721
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added allocation metrics for allocation time.
> 
> 
> Diffs
> -
> 
>   docs/monitoring.md 827f7073204fcf8575ca980a5571c8be4f5e4110 
>   src/master/allocator/mesos/hierarchical.cpp 
> 70291075c00a9a557529c2562dedcfc6c6c3ec32 
>   src/master/allocator/mesos/metrics.hpp PRE-CREATION 
>   src/master/allocator/mesos/metrics.cpp PRE-CREATION 
>   src/tests/hierarchical_allocator_tests.cpp 
> 3e4ad31925e1b815a74d67fa3962d23fa5bc89d1 
> 
> Diff: https://reviews.apache.org/r/43882/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
> 
>

Re: Review Request 43882: Added allocation metrics for allocation time.

2016-03-03 Thread Benjamin Bannier 

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

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


Review request for mesos, Alexander Rukletsov and Ben Mahler.


Changes
---

Addressed comments form Alex.


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


Repository: mesos


Description
---

Added allocation metrics for allocation time.


Diffs (updated)
-

  docs/monitoring.md 827f7073204fcf8575ca980a5571c8be4f5e4110 
  src/master/allocator/mesos/hierarchical.cpp 
70291075c00a9a557529c2562dedcfc6c6c3ec32 
  src/master/allocator/mesos/metrics.hpp PRE-CREATION 
  src/master/allocator/mesos/metrics.cpp PRE-CREATION 
  src/tests/hierarchical_allocator_tests.cpp 
3e4ad31925e1b815a74d67fa3962d23fa5bc89d1 

Diff: https://reviews.apache.org/r/43882/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

Re: Review Request 43882: Added allocation metrics for allocation time.

2016-03-03 Thread Benjamin Bannier 

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

(Updated March 3, 2016, 2:32 p.m.)


Review request for mesos, Alexander Rukletsov and Ben Mahler.


Changes
---

Tidied up includes.


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


Repository: mesos


Description
---

Added allocation metrics for allocation time.


Diffs (updated)
-

  docs/monitoring.md 827f7073204fcf8575ca980a5571c8be4f5e4110 
  src/master/allocator/mesos/hierarchical.cpp 
70291075c00a9a557529c2562dedcfc6c6c3ec32 
  src/master/allocator/mesos/metrics.hpp PRE-CREATION 
  src/master/allocator/mesos/metrics.cpp PRE-CREATION 
  src/tests/hierarchical_allocator_tests.cpp 
3e4ad31925e1b815a74d67fa3962d23fa5bc89d1 

Diff: https://reviews.apache.org/r/43882/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

Re: Review Request 43882: Added allocation metrics for allocation time.

2016-03-02 Thread Benjamin Bannier 

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

(Updated March 2, 2016, 4:43 p.m.)


Review request for mesos, Alexander Rukletsov and Ben Mahler.


Changes
---

Rebased, and minor style fixes.


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


Repository: mesos


Description
---

Added allocation metrics for allocation time.


Diffs (updated)
-

  docs/monitoring.md 827f7073204fcf8575ca980a5571c8be4f5e4110 
  src/master/allocator/mesos/hierarchical.cpp 
24fa50f62dec10ed43089297473cc386d6ba2f78 
  src/master/allocator/mesos/metrics.hpp PRE-CREATION 
  src/master/allocator/mesos/metrics.cpp PRE-CREATION 
  src/tests/hierarchical_allocator_tests.cpp 
5f771f02db9bd098f3cd36730cd84bf2f5e87a33 

Diff: https://reviews.apache.org/r/43882/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

Re: Review Request 43882: Added allocation metrics for allocation time.

2016-03-02 Thread Benjamin Bannier 


> On March 1, 2016, 12:48 p.m., Alexander Rukletsov wrote:
> > src/tests/hierarchical_allocator_tests.cpp, line 2577
> > 
> >
> > Why don't you need to convert to `JSON::Number` first?

`time` is a `JSON::Number`; it was created from the `JSON::Value` `value` above.


> On March 1, 2016, 12:48 p.m., Alexander Rukletsov wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 1216-1219
> > 
> >
> > As per our offline discussion, let's pull it out to libprocess. Maybe 
> > even create a ticket for a sweep change to use this new helper in relevant 
> > places?

See https://reviews.apache.org/r/44261/.


> On March 1, 2016, 12:48 p.m., Alexander Rukletsov wrote:
> > src/tests/hierarchical_allocator_tests.cpp, line 2568
> > 
> >
> > Here two event-based allocation happen, but you are only interested in 
> > one. Could you please describe your intention, so that folks understand why 
> > it is fine? Thanks.

I added a comment.


- Benjamin


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


On March 2, 2016, 11:34 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43882/
> ---
> 
> (Updated March 2, 2016, 11:34 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
> 
> 
> Bugs: MESOS-4721
> https://issues.apache.org/jira/browse/MESOS-4721
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added allocation metrics for allocation time.
> 
> 
> Diffs
> -
> 
>   docs/monitoring.md 827f7073204fcf8575ca980a5571c8be4f5e4110 
>   src/master/allocator/mesos/hierarchical.cpp 
> 24fa50f62dec10ed43089297473cc386d6ba2f78 
>   src/master/allocator/mesos/metrics.hpp PRE-CREATION 
>   src/tests/hierarchical_allocator_tests.cpp 
> 5f771f02db9bd098f3cd36730cd84bf2f5e87a33 
> 
> Diff: https://reviews.apache.org/r/43882/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
> 
>

Re: Review Request 43882: Added allocation metrics for allocation time.

2016-03-02 Thread Benjamin Bannier 

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

(Updated March 2, 2016, 11:34 a.m.)


Review request for mesos, Alexander Rukletsov and Ben Mahler.


Changes
---

Styles fixes.


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


Repository: mesos


Description
---

Added allocation metrics for allocation time.


Diffs (updated)
-

  docs/monitoring.md 827f7073204fcf8575ca980a5571c8be4f5e4110 
  src/master/allocator/mesos/hierarchical.cpp 
24fa50f62dec10ed43089297473cc386d6ba2f78 
  src/master/allocator/mesos/metrics.hpp PRE-CREATION 
  src/tests/hierarchical_allocator_tests.cpp 
5f771f02db9bd098f3cd36730cd84bf2f5e87a33 

Diff: https://reviews.apache.org/r/43882/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

Re: Review Request 43882: Added allocation metrics for allocation time.

2016-03-01 Thread Alexander Rukletsov 

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




src/master/allocator/mesos/hierarchical.cpp (lines 1216 - 1219)


As per our offline discussion, let's pull it out to libprocess. Maybe even 
create a ticket for a sweep change to use this new helper in relevant places?



src/tests/hierarchical_allocator_tests.cpp (lines 2557 - 2558)


Blank line.



src/tests/hierarchical_allocator_tests.cpp (line 2559)


You can remove this blank line.



src/tests/hierarchical_allocator_tests.cpp (line 2568)


Here two event-based allocation happen, but you are only interested in one. 
Could you please describe your intention, so that folks understand why it is 
fine? Thanks.



src/tests/hierarchical_allocator_tests.cpp (line 2577)


Why don't you need to convert to `JSON::Number` first?


- Alexander Rukletsov


On Feb. 29, 2016, 7:59 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43882/
> ---
> 
> (Updated Feb. 29, 2016, 7:59 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
> 
> 
> Bugs: MESOS-4721
> https://issues.apache.org/jira/browse/MESOS-4721
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added allocation metrics for allocation time.
> 
> 
> 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/43882/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
> 
>

Re: Review Request 43882: Added allocation metrics for allocation time.

2016-02-29 Thread Alexander Rojas 


> On Feb. 27, 2016, 2:11 a.m., Alexander Rojas wrote:
> > src/master/allocator/mesos/hierarchical.hpp, line 360
> > 
> >
> > There is a second parameter in the `Timer` object which allows you to 
> > specify a window, once this window is enabled we can get statistics of the 
> > run beyond how long the last allocation took place (note that allocation 
> > varies a lot since sometimes it goes through a few elements and other times 
> > goes throught the whole cluster).
> 
> Benjamin Bannier wrote:
> Yes, the timer I added here can only be sampled and provides no stats. Do 
> you have a suggestion how many `allocation_intervals` the window should be?

I'm not really sure, I think it would be better if you consult with your 
shepherd on this matter.


- Alexander


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


On Feb. 29, 2016, 8:59 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43882/
> ---
> 
> (Updated Feb. 29, 2016, 8:59 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
> 
> 
> Bugs: MESOS-4721
> https://issues.apache.org/jira/browse/MESOS-4721
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added allocation metrics for allocation time.
> 
> 
> 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/43882/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
> 
>

Re: Review Request 43882: Added allocation metrics for allocation time.

2016-02-29 Thread Benjamin Bannier 

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

(Updated Feb. 29, 2016, 8:59 p.m.)


Review request for mesos, Alexander Rukletsov and Ben Mahler.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Added allocation metrics for allocation time.


Diffs (updated)
-

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

Re: Review Request 43882: Added allocation metrics for allocation time.

2016-02-29 Thread Alexander Rojas 


> On Feb. 27, 2016, 2:11 a.m., Alexander Rojas wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 1220
> > 
> >
> > Add a `CHECK_NOT_NULL`
> 
> Benjamin Bannier wrote:
> `timer` is initialized from a ref in the line above, so I think it can 
> never be null by construction.

you are right. My bad.


- Alexander


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


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/43882/
> ---
> 
> (Updated Feb. 28, 2016, 10:28 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
> 
> 
> Bugs: MESOS-4721
> https://issues.apache.org/jira/browse/MESOS-4721
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added allocation metrics for allocation time.
> 
> 
> 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/43882/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
> 
>

Re: Review Request 43882: Added allocation metrics for allocation time.

2016-02-28 Thread Benjamin Bannier 


> On Feb. 25, 2016, 5:47 p.m., Klaus Ma wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 1207
> > 
> >
> > It seems a general functional class, can we move it into 
> > `process/metrics`?

I currently can only see a handful of uses of `process::metrics::Timer`, non of 
which would benefit from this helper yet. Let's pull it out once we see a 
general need.


- Benjamin


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


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/43882/
> ---
> 
> (Updated Feb. 28, 2016, 10:28 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
> 
> 
> Bugs: MESOS-4721
> https://issues.apache.org/jira/browse/MESOS-4721
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added allocation metrics for allocation time.
> 
> 
> 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/43882/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
> 
>

Re: Review Request 43882: Added allocation metrics for allocation time.

2016-02-28 Thread Benjamin Bannier 

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

(Updated Feb. 28, 2016, 10:28 p.m.)


Review request for mesos, Alexander Rukletsov and Ben Mahler.


Changes
---

Mesos style.


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


Repository: mesos


Description
---

Added allocation metrics for allocation time.


Diffs (updated)
-

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

Re: Review Request 43882: Added allocation metrics for allocation time.

2016-02-26 Thread Alexander Rojas 

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




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


There is a second parameter in the `Timer` object which allows you to 
specify a window, once this window is enabled we can get statistics of the run 
beyond how long the last allocation took place (note that allocation varies a 
lot since sometimes it goes through a few elements and other times goes 
throught the whole cluster).



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


Add a `CHECK_NOT_NULL`



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


While I prefer this kind of constructors. I think I rather go for 
consistency and Mesos code is rather consistent on using parenthesis.

If you look at this file, this will be the only instance of this style (not 
including initializer lists that is).


- Alexander Rojas


On Feb. 26, 2016, 6:01 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43882/
> ---
> 
> (Updated Feb. 26, 2016, 6:01 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
> 
> 
> Bugs: MESOS-4721
> https://issues.apache.org/jira/browse/MESOS-4721
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added allocation metrics for allocation time.
> 
> 
> Diffs
> -
> 
>   docs/monitoring.md 323d01d99456a71bd384faf186264e3fc4bf2207 
>   src/master/allocator/mesos/hierarchical.hpp 
> 3043888630b066505410d3b32c5b3f813cc458c1 
>   src/master/allocator/mesos/hierarchical.cpp 
> 5ef29f26ec8071f79c2f4f78dbe2bb0a613cc92d 
>   src/tests/hierarchical_allocator_tests.cpp 
> 5f771f02db9bd098f3cd36730cd84bf2f5e87a33 
> 
> Diff: https://reviews.apache.org/r/43882/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
> 
>

Re: Review Request 43882: Added allocation metrics for allocation time.

2016-02-26 Thread Benjamin Bannier 

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

(Updated Feb. 26, 2016, 6:01 p.m.)


Review request for mesos, Alexander Rukletsov and Ben Mahler.


Changes
---

Rebased and get metrics without a helper.


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


Repository: mesos


Description
---

Added allocation metrics for allocation time.


Diffs (updated)
-

  docs/monitoring.md 323d01d99456a71bd384faf186264e3fc4bf2207 
  src/master/allocator/mesos/hierarchical.hpp 
3043888630b066505410d3b32c5b3f813cc458c1 
  src/master/allocator/mesos/hierarchical.cpp 
5ef29f26ec8071f79c2f4f78dbe2bb0a613cc92d 
  src/tests/hierarchical_allocator_tests.cpp 
5f771f02db9bd098f3cd36730cd84bf2f5e87a33 

Diff: https://reviews.apache.org/r/43882/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

Re: Review Request 43882: Added allocation metrics for allocation time.

2016-02-25 Thread Klaus Ma 

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




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


It seems a general functional class, can we move it into `process/metrics`?


- Klaus Ma


On Feb. 25, 2016, 7:37 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43882/
> ---
> 
> (Updated Feb. 25, 2016, 7:37 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
> 
> 
> Bugs: MESOS-4721
> https://issues.apache.org/jira/browse/MESOS-4721
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added allocation metrics for allocation time.
> 
> 
> Diffs
> -
> 
>   docs/monitoring.md 323d01d99456a71bd384faf186264e3fc4bf2207 
>   src/master/allocator/mesos/hierarchical.hpp 
> 3043888630b066505410d3b32c5b3f813cc458c1 
>   src/master/allocator/mesos/hierarchical.cpp 
> 5ef29f26ec8071f79c2f4f78dbe2bb0a613cc92d 
>   src/tests/hierarchical_allocator_tests.cpp 
> 5f771f02db9bd098f3cd36730cd84bf2f5e87a33 
> 
> Diff: https://reviews.apache.org/r/43882/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
> 
>

Re: Review Request 43882: Added allocation metrics for allocation time.

2016-02-25 Thread Benjamin Bannier 

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

(Updated Feb. 25, 2016, 12:37 p.m.)


Review request for mesos, Alexander Rukletsov and Ben Mahler.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Added allocation metrics for allocation time.


Diffs (updated)
-

  docs/monitoring.md 323d01d99456a71bd384faf186264e3fc4bf2207 
  src/master/allocator/mesos/hierarchical.hpp 
3043888630b066505410d3b32c5b3f813cc458c1 
  src/master/allocator/mesos/hierarchical.cpp 
5ef29f26ec8071f79c2f4f78dbe2bb0a613cc92d 
  src/tests/hierarchical_allocator_tests.cpp 
5f771f02db9bd098f3cd36730cd84bf2f5e87a33 

Diff: https://reviews.apache.org/r/43882/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

Re: Review Request 43882: Added allocation metrics for allocation time.

2016-02-25 Thread Benjamin Bannier 

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

(Updated Feb. 25, 2016, 12:01 p.m.)


Review request for mesos, Alexander Rukletsov and Ben Mahler.


Changes
---

Added documentation and fixed ws.


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


Repository: mesos


Description
---

Added allocation metrics for allocation time.


Diffs (updated)
-

  docs/monitoring.md 323d01d99456a71bd384faf186264e3fc4bf2207 
  src/master/allocator/mesos/hierarchical.hpp 
3043888630b066505410d3b32c5b3f813cc458c1 
  src/master/allocator/mesos/hierarchical.cpp 
5ef29f26ec8071f79c2f4f78dbe2bb0a613cc92d 
  src/tests/hierarchical_allocator_tests.cpp 
5f771f02db9bd098f3cd36730cd84bf2f5e87a33 

Diff: https://reviews.apache.org/r/43882/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

Re: Review Request 43882: Added allocation metrics for allocation time.

2016-02-24 Thread Benjamin Bannier 

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

(Updated Feb. 24, 2016, 11:49 a.m.)


Review request for mesos, Alexander Rukletsov and Ben Mahler.


Changes
---

Just whitespace (no functional changes)


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


Repository: mesos


Description
---

Added allocation metrics for allocation time.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.hpp 
3043888630b066505410d3b32c5b3f813cc458c1 
  src/master/allocator/mesos/hierarchical.cpp 
5ef29f26ec8071f79c2f4f78dbe2bb0a613cc92d 
  src/tests/hierarchical_allocator_tests.cpp 
5f771f02db9bd098f3cd36730cd84bf2f5e87a33 

Diff: https://reviews.apache.org/r/43882/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

Re: Review Request 43882: Added allocation metrics for allocation time.

2016-02-24 Thread Benjamin Bannier 

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

(Updated Feb. 24, 2016, 11:23 a.m.)


Review request for mesos, Alexander Rukletsov and Ben Mahler.


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


Repository: mesos


Description
---

Added allocation metrics for allocation time.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.hpp 
3043888630b066505410d3b32c5b3f813cc458c1 
  src/master/allocator/mesos/hierarchical.cpp 
5ef29f26ec8071f79c2f4f78dbe2bb0a613cc92d 
  src/tests/hierarchical_allocator_tests.cpp 
5f771f02db9bd098f3cd36730cd84bf2f5e87a33 

Diff: https://reviews.apache.org/r/43882/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

Review Request 43882: Added allocation metrics for allocation time.

2016-02-23 Thread Benjamin Bannier 

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

Review request for mesos, Alexander Rukletsov and Ben Mahler.


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


Repository: mesos


Description
---

Added allocation metrics for allocation time.


Diffs
-

  src/master/allocator/mesos/hierarchical.hpp 
0d39d3f3b5f4ff7f62f9de7200d062845c71818a 
  src/master/allocator/mesos/hierarchical.cpp 
5ef29f26ec8071f79c2f4f78dbe2bb0a613cc92d 
  src/tests/hierarchical_allocator_tests.cpp 
1af5c9870430eb05ca0b19ece0ca2957a05093ff 

Diff: https://reviews.apache.org/r/43882/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