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

2016-03-04 Thread Alexander Rojas 

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



Most of my comments are based on an email @alex-mesos sent today, in which he 
suggests removing metrics related code from the allocator and into the 
`Metrics` class, which is an idea I strongly support. If we go that way, I 
would double down into that and move _all_ related code into metrics.


src/master/allocator/mesos/hierarchical.cpp (lines 415 - 420)


It just occurred to me, since @alex-mesos mentioned moving code related to 
metrics to the `Metrics` class, and they are closely related, why not do the 
`foreach` there in a method `Metrics::createGaugesForScalars(const 
std::set& names)`



src/master/allocator/mesos/hierarchical.cpp (lines 528 - 533)


ditto



src/master/allocator/mesos/hierarchical.cpp (lines 1705 - 1730)


I wonder if we can move this code to the `Metrics` class. It can be in the 
code that actually calls this method… Unless we plan to use this functions for 
other purposes than updating the gauges.



src/master/allocator/mesos/metrics.hpp (line 43)


A reference perhaps? I don't understand why is a const pointer better in 
this case.


Over

- Alexander Rojas


On March 4, 2016, 5:01 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43880/
> ---
> 
> (Updated March 4, 2016, 5:01 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
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   docs/monitoring.md 827f7073204fcf8575ca980a5571c8be4f5e4110 
>   src/master/allocator/mesos/hierarchical.hpp 
> 3043888630b066505410d3b32c5b3f813cc458c1 
>   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/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
> 
>

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

2016-03-04 Thread Benjamin Bannier 


> On March 4, 2016, 2:05 p.m., Alexander Rukletsov wrote:
> > src/master/allocator/mesos/metrics.hpp, line 27
> > 
> >
> > Kill this line.

As if it never was there now.


> On March 4, 2016, 2:05 p.m., Alexander Rukletsov wrote:
> > src/master/allocator/mesos/metrics.cpp, lines 61-63
> > 
> >
> > Does it make sense to store the pointer to 
> > `HierarchicalAllocatorProcess` in the `Metrics` instance and remove the 
> > first parameter here? The value of this approach will be more evident when 
> > we will have more helpers later.

Yes, that makes sense. This puts some more restrictions on how an allocator 
`Metrics` can be used. For now I made the class e.g., non-copyable and added a 
`NOTE` on the lifetime dependency with the `HierarchicalAllocatorProcess`.


> On March 4, 2016, 2:05 p.m., Alexander Rukletsov wrote:
> > src/tests/hierarchical_allocator_tests.cpp, line 2406
> > 
> >
> > s/users/frameworks

Done.


> On March 4, 2016, 2:05 p.m., Alexander Rukletsov wrote:
> > src/tests/hierarchical_allocator_tests.cpp, lines 2420-2424
> > 
> >
> > Can you move it up where you check allocated/* metrics? Adding a 
> > framework shouldn't influence this counter.

I think this moved down here in some cleanup, I moved it back up.


> On March 4, 2016, 2:05 p.m., Alexander Rukletsov wrote:
> > src/tests/hierarchical_allocator_tests.cpp, lines 2449-2453
> > 
> >
> > I think resources will be offered to `framework2`. But is it actually 
> > important? I think you can kill this comment altogether (also the similar 
> > one above), and add a short one around `settle()`.

Done.


> On March 4, 2016, 2:05 p.m., Alexander Rukletsov wrote:
> > src/tests/hierarchical_allocator_tests.cpp, line 2463
> > 
> >
> > Feel free to kill this line.

It didn't leave a trace.


> On March 4, 2016, 2:05 p.m., Alexander Rukletsov wrote:
> > src/tests/hierarchical_allocator_tests.cpp, line 2491
> > 
> >
> > /cluster/cluster state/

Stated as such.


- Benjamin


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


On March 4, 2016, 5:01 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43880/
> ---
> 
> (Updated March 4, 2016, 5:01 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
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   docs/monitoring.md 827f7073204fcf8575ca980a5571c8be4f5e4110 
>   src/master/allocator/mesos/hierarchical.hpp 
> 3043888630b066505410d3b32c5b3f813cc458c1 
>   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/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
> 
>

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

2016-03-04 Thread Benjamin Bannier 

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

(Updated March 4, 2016, 5:01 p.m.)


Review request for mesos, Alexander Rukletsov and Ben Mahler.


Changes
---

Adressed comments from Alex.


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


Repository: mesos


Description (updated)
---

See summary.


Diffs (updated)
-

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

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

2016-03-04 Thread Alexander Rukletsov 

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


Fix it, then Ship it!





src/master/allocator/mesos/metrics.hpp (line 27)


Kill this line.



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


Does it make sense to store the pointer to `HierarchicalAllocatorProcess` 
in the `Metrics` instance and remove the first parameter here? The value of 
this approach will be more evident when we will have more helpers later.



src/tests/hierarchical_allocator_tests.cpp (line 2406)


s/users/frameworks



src/tests/hierarchical_allocator_tests.cpp (lines 2420 - 2424)


Can you move it up where you check allocated/* metrics? Adding a framework 
shouldn't influence this counter.



src/tests/hierarchical_allocator_tests.cpp (lines 2449 - 2453)


I think resources will be offered to `framework2`. But is it actually 
important? I think you can kill this comment altogether (also the similar one 
above), and add a short one around `settle()`.



src/tests/hierarchical_allocator_tests.cpp (line 2463)


Feel free to kill this line.



src/tests/hierarchical_allocator_tests.cpp (line 2491)


/cluster/cluster state/


- 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/43880/
> ---
> 
> (Updated March 3, 2016, 4:17 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 
> 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/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
> 
>

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

2016-03-04 Thread Alexander Rukletsov 


> On March 3, 2016, 3:04 p.m., Alexander Rukletsov wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 1711-1712
> > 
> >
> > My intuition is that using `allocationScalarQuantities` will be more 
> > efficient. However, we should expose known clients from the sorter. What do 
> > you think?
> 
> Benjamin Bannier wrote:
> As for efficiency, I don't think there's an issue here; we just create a 
> `Value::Scalar` which I think cannot be avoided, and e.g., never create 
> temporary `Resources`. Would we use `allocationScalarQuantities`, we'd always 
> be forced to create temporary `Resources` (which in an ideal world could be 
> optimized away again).

I was thinking that iterating over active roles should be cheaper than over all 
agents. But it's fine to leave it as is and come back if we observe some 
performance issues.


- Alexander


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


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/43880/
> ---
> 
> (Updated March 3, 2016, 4:17 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 
> 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/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
> 
>

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

2016-03-03 Thread Benjamin Bannier 


> On March 3, 2016, 4:04 p.m., Alexander Rukletsov wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 1711-1712
> > 
> >
> > My intuition is that using `allocationScalarQuantities` will be more 
> > efficient. However, we should expose known clients from the sorter. What do 
> > you think?

As for efficiency, I don't think there's an issue here; we just create a 
`Value::Scalar` which I think cannot be avoided, and e.g., never create 
temporary `Resources`. Would we use `allocationScalarQuantities`, we'd always 
be forced to create temporary `Resources` (which in an ideal world could be 
optimized away again).


> On March 3, 2016, 4:04 p.m., Alexander Rukletsov wrote:
> > src/master/allocator/mesos/metrics.cpp, lines 78-81
> > 
> >
> > Why not using C++ lambdas instead of `bind`?

Good idea.


> On March 3, 2016, 4:04 p.m., Alexander Rukletsov wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 540-545
> > 
> >
> > Let's call it right after `CHECK_*`s

Done.


> On March 3, 2016, 4:04 p.m., Alexander Rukletsov wrote:
> > src/master/allocator/mesos/hierarchical.hpp, lines 447-450
> > 
> >
> > Do you still need it?

You are right, not needed anymore and also not implemented.


> On March 3, 2016, 4:04 p.m., Alexander Rukletsov wrote:
> > docs/monitoring.md, line 879
> > 
> >
> > Maybe "Allocated resources of kind KIND"?

Done, also fixed the doc for `allocator/total` below.


- Benjamin


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


On March 3, 2016, 5:17 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43880/
> ---
> 
> (Updated March 3, 2016, 5:17 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 
> 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/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
> 
>

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

2016-03-03 Thread Benjamin Bannier 

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

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


Review request for mesos, Alexander Rukletsov and Ben Mahler.


Changes
---

Addressed comments form Alex.


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


Repository: mesos


Description (updated)
---

Added allocator metrics for total and allocated scalar resources.


Diffs (updated)
-

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

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

2016-03-03 Thread Alexander Rukletsov 

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




docs/monitoring.md (line 879)


Maybe "Allocated resources of kind KIND"?



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


Maybe move this comment into r/44260/?



src/master/allocator/mesos/hierarchical.hpp (lines 447 - 450)


Do you still need it?



src/master/allocator/mesos/hierarchical.cpp (lines 540 - 545)


Let's call it right after `CHECK_*`s



src/master/allocator/mesos/hierarchical.cpp (lines 1711 - 1712)


My intuition is that using `allocationScalarQuantities` will be more 
efficient. However, we should expose known clients from the sorter. What do you 
think?



src/master/allocator/mesos/metrics.cpp (lines 78 - 81)


Why not using C++ lambdas instead of `bind`?


- Alexander Rukletsov


On March 3, 2016, 1:12 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43880/
> ---
> 
> (Updated March 3, 2016, 1:12 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
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   docs/monitoring.md 827f7073204fcf8575ca980a5571c8be4f5e4110 
>   src/master/allocator/mesos/hierarchical.hpp 
> 3043888630b066505410d3b32c5b3f813cc458c1 
>   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/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
> 
>

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

2016-03-03 Thread Alexander Rukletsov 


> On March 1, 2016, 11:48 a.m., Alexander Rukletsov wrote:
> > src/tests/hierarchical_allocator_tests.cpp, line 2406
> > 
> >
> > I believe the actual value in JSON is double, right?
> 
> Benjamin Bannier wrote:
> This is actually a `JSON::Value`; the corresponding `operator==` which 
> will convert the lhs to a `JSON::Value` as well.

Does it make sense to say `0.` to hint the compiler create a `JSON::Value` with 
a double? It looks like `Value::ContainmentComparator` will do the job anyway, 
but it feels cleaner to me. What do you think?


- Alexander


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


On March 3, 2016, 1:12 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43880/
> ---
> 
> (Updated March 3, 2016, 1:12 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
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   docs/monitoring.md 827f7073204fcf8575ca980a5571c8be4f5e4110 
>   src/master/allocator/mesos/hierarchical.hpp 
> 3043888630b066505410d3b32c5b3f813cc458c1 
>   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/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
> 
>

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

2016-03-03 Thread Benjamin Bannier 

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

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


Review request for mesos, Alexander Rukletsov and Ben Mahler.


Changes
---

Rebased for changes in `master`, tidied up includes.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

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

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

2016-03-02 Thread Benjamin Bannier 

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

(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-4720
https://issues.apache.org/jira/browse/MESOS-4720


Repository: mesos


Description (updated)
---

See summary.


Diffs (updated)
-

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

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

2016-03-02 Thread Benjamin Bannier 

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

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


Review request for mesos, Alexander Rukletsov and Ben Mahler.


Changes
---

Backtickified & commentified.


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


Repository: mesos


Description
---

Added allocator metrics for total and allocated scalar resources.


Diffs (updated)
-

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

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

2016-03-02 Thread Benjamin Bannier 


> On Feb. 29, 2016, 11:48 p.m., Alexander Rojas wrote:
> > 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 `metrics.allocated`.
> > 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:
> > 
> > ```c++
> > Deferred totalGenerator = 
> >   process::defer(self(), [this, name]() {
> > Option value =
> >   this->roleSorter->totalScalars().get(name);
> > return value.isSome() ? value.get().value() ? 0.0;
> >   });
> > 
> > metrics.allocated.put(
> > name,
> > process::metrics::Gauge(
> > strings::join("/", "allocator/allocated", name),
> > totalGenerator));
> > 
> > ```

To make this more readable and obvious I introduced getters analoguous to 
`HierarchicalAllocatorProcess::_event_queue_dispatches`.


- Benjamin


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


On March 2, 2016, 11:35 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43880/
> ---
> 
> (Updated March 2, 2016, 11:35 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 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/master/allocator/mesos/metrics.hpp PRE-CREATION 
>   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
> 
>

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

2016-03-02 Thread Benjamin Bannier 


> On March 1, 2016, 12:48 p.m., Alexander Rukletsov wrote:
> > src/tests/hierarchical_allocator_tests.cpp, line 2406
> > 
> >
> > I believe the actual value in JSON is double, right?

This is actually a `JSON::Value`; the corresponding `operator==` which will 
convert the lhs to a `JSON::Value` as well.


> On March 1, 2016, 12:48 p.m., Alexander Rukletsov wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 402
> > 
> >
> > Can we factor this out to a separate file? I would like to keep this 
> > file as small and readable as possible.

I moved this into the `Metrics` (which also live in a different file now).


> On March 1, 2016, 12:48 p.m., Alexander Rukletsov wrote:
> > src/tests/hierarchical_allocator_tests.cpp, lines 2445-2446
> > 
> >
> > How it is different to the previous block?

Thanks, my `Control-C Control-V` key must have been stuck ;)


> On March 1, 2016, 12:48 p.m., Alexander Rukletsov wrote:
> > src/tests/hierarchical_allocator_tests.cpp, line 2476
> > 
> >
> > Why this math?

I added some comments.


- Benjamin


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


On March 2, 2016, 11:35 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43880/
> ---
> 
> (Updated March 2, 2016, 11:35 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 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/master/allocator/mesos/metrics.hpp PRE-CREATION 
>   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
> 
>

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

2016-03-02 Thread Benjamin Bannier 

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

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


Review request for mesos, Alexander Rukletsov and Ben Mahler.


Changes
---

Style fixes.


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


Repository: mesos


Description
---

Added allocator metrics for total and allocated scalar resources.


Diffs (updated)
-

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

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

2016-03-01 Thread Alexander Rukletsov 

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




docs/monitoring.md (lines 881 - 883)


Same question as in the previous review: does it make sense to put them all 
on the same line?



src/master/allocator/mesos/hierarchical.hpp (lines 283 - 284)


Blank line, please!

We usually separate loops with blank lines from the surrounding code, 
except when it tightly related to the loop, which is not the case here, right?



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


Can we factor this out to a separate file? I would like to keep this file 
as small and readable as possible.



src/tests/hierarchical_allocator_tests.cpp (line 2406)


I believe the actual value in JSON is double, right?



src/tests/hierarchical_allocator_tests.cpp (lines 2419 - 2420)


Let's insert a blank line here or make the comment trailing.



src/tests/hierarchical_allocator_tests.cpp (line 2423)


Don't forget to backtick variables! Here it's `agent1`.



src/tests/hierarchical_allocator_tests.cpp (lines 2445 - 2446)


How it is different to the previous block?



src/tests/hierarchical_allocator_tests.cpp (line 2476)


Why this math?


- Alexander Rukletsov


On Feb. 28, 2016, 9: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, 9: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
> 
>

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

2016-02-29 Thread Alexander Rojas 

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




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 
`metrics.allocated`.
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:

```c++
Deferred totalGenerator = 
  process::defer(self(), [this, name]() {
Option value =
  this->roleSorter->totalScalars().get(name);
return value.isSome() ? value.get().value() ? 0.0;
  });

metrics.allocated.put(
name,
process::metrics::Gauge(
strings::join("/", "allocator/allocated", name),
totalGenerator));

```



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


Same as above.



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


s/result/allocated/


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

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

2016-02-28 Thread Benjamin Bannier 

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


Changes
---

Did not move definition around, and rebased.


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


Repository: mesos


Description
---

Added allocator metrics for total and allocated scalar resources.


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

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

2016-02-26 Thread Benjamin Bannier 

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

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


Review request for mesos, Alexander Rukletsov and Ben Mahler.


Changes
---

Rebased and get metrics without a helper.


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


Repository: mesos


Description
---

Added allocator metrics for total and allocated scalar resources.


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

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

2016-02-25 Thread Benjamin Bannier 

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

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


Review request for mesos, Alexander Rukletsov and Ben Mahler.


Changes
---

Added documentation, fixed stray `FIXME`s and ws, and stayed away from `const` 
locals.


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


Repository: mesos


Description
---

Added allocator metrics for total and allocated scalar resources.


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

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

2016-02-24 Thread Benjamin Bannier 

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

(Updated Feb. 24, 2016, 8:24 p.m.)


Review request for mesos, Alexander Rukletsov and Ben Mahler.


Changes
---

Addressed klaus1982's comments.


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


Repository: mesos


Description
---

Added allocator metrics for total and allocated scalar resources.


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

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

2016-02-24 Thread Benjamin Bannier 


> On Feb. 24, 2016, 5:09 p.m., Klaus Ma wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 428
> > 
> >
> > Not yours, but I think we need to update roleSorter when slave 
> > active/deactive.
> > 
> > Filed https://issues.apache.org/jira/browse/MESOS-4755 to trace this.

Dropping this as it is already tracked separately.


> On Feb. 24, 2016, 5:09 p.m., Klaus Ma wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 416
> > 
> >
> > I think we also need to update it in `updateSlave`; if normal resources 
> > of agent is "cpus:16" and Estimator report "mem:10", we'll miss mem in 
> > total metrics.

Good catch, the interface certainly allows new resource kinds appearing via 
`updateSlave`.


- Benjamin


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


On Feb. 24, 2016, 11:58 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43880/
> ---
> 
> (Updated Feb. 24, 2016, 11:58 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 allocator metrics for total and allocated scalar resources.
> 
> 
> Diffs
> -
> 
>   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/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
> 
>

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

2016-02-24 Thread Klaus Ma 

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




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


I think we also need to update it in `updateSlave`; if normal resources of 
agent is "cpus:16" and Estimator report "mem:10", we'll miss mem in total 
metrics.



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


Not yours, but I think we need to update roleSorter when slave 
active/deactive.

Filed https://issues.apache.org/jira/browse/MESOS-4755 to trace this.



src/tests/hierarchical_allocator_tests.cpp (line 2457)


Can we move the comments above the code and move `getMetric()` into one 
line?


- Klaus Ma


On Feb. 24, 2016, 6:58 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43880/
> ---
> 
> (Updated Feb. 24, 2016, 6:58 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
> -
> 
>   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/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
> 
>

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

2016-02-24 Thread Benjamin Bannier 


> On Feb. 24, 2016, 1:26 a.m., Guangya Liu wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 432-444
> > 
> >
> > The allocatd may keeps changing due the resource offer and resource 
> > recovery, I think that this logic should be in `allocate(slaveIds)`?
> 
> Benjamin Bannier wrote:
> We do not read out `slave.allocated` here, we just install a callback 
> doing that. The calculation of the actual value is triggered if the metrics 
> endpoint is hit. Or am I missing an implicit value copy here?
> 
> Guangya Liu wrote:
> Yes, you are right. So can you please add some comments here to clairfy 
> that you install a callback to get the allocated resources?
> 
> Benjamin Bannier wrote:
> In addition to the existing one on l.413?
> 
> Guangya Liu wrote:
> I think that line 421 maybe better? what about clarify the two install 
> callbacks there?

I expanded the comment on l.413 to give more detail.


- Benjamin


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


On Feb. 24, 2016, 11:58 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43880/
> ---
> 
> (Updated Feb. 24, 2016, 11:58 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 allocator metrics for total and allocated scalar resources.
> 
> 
> Diffs
> -
> 
>   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/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
> 
>

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

2016-02-24 Thread Benjamin Bannier 

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

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


Review request for mesos, Alexander Rukletsov and Ben Mahler.


Changes
---

Expanded comment to detail what kind of gauges we are adding.


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


Repository: mesos


Description
---

Added allocator metrics for total and allocated scalar resources.


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

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

2016-02-24 Thread Benjamin Bannier 

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

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


Review request for mesos, Alexander Rukletsov and Ben Mahler.


Changes
---

Just whitespace (no functional changes)


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


Repository: mesos


Description
---

Added allocator metrics for total and allocated scalar resources.


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

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

2016-02-24 Thread Benjamin Bannier 

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

(Updated Feb. 24, 2016, 11:22 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 allocator metrics for total and allocated scalar resources.


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

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

2016-02-24 Thread Guangya Liu 


> On 二月 24, 2016, 12:26 a.m., Guangya Liu wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 432-444
> > 
> >
> > The allocatd may keeps changing due the resource offer and resource 
> > recovery, I think that this logic should be in `allocate(slaveIds)`?
> 
> Benjamin Bannier wrote:
> We do not read out `slave.allocated` here, we just install a callback 
> doing that. The calculation of the actual value is triggered if the metrics 
> endpoint is hit. Or am I missing an implicit value copy here?
> 
> Guangya Liu wrote:
> Yes, you are right. So can you please add some comments here to clairfy 
> that you install a callback to get the allocated resources?
> 
> Benjamin Bannier wrote:
> In addition to the existing one on l.413?

I think that line 421 maybe better? what about clarify the two install 
callbacks there?


- Guangya


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


On 二月 24, 2016, 8:46 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43880/
> ---
> 
> (Updated 二月 24, 2016, 8:46 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 allocator metrics for total and allocated scalar resources.
> 
> 
> Diffs
> -
> 
>   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/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
> 
>

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

2016-02-24 Thread Benjamin Bannier 


> On Feb. 24, 2016, 1:26 a.m., Guangya Liu wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 432-444
> > 
> >
> > The allocatd may keeps changing due the resource offer and resource 
> > recovery, I think that this logic should be in `allocate(slaveIds)`?
> 
> Benjamin Bannier wrote:
> We do not read out `slave.allocated` here, we just install a callback 
> doing that. The calculation of the actual value is triggered if the metrics 
> endpoint is hit. Or am I missing an implicit value copy here?
> 
> Guangya Liu wrote:
> Yes, you are right. So can you please add some comments here to clairfy 
> that you install a callback to get the allocated resources?

In addition to the existing one on l.413?


- Benjamin


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


On Feb. 24, 2016, 9:46 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43880/
> ---
> 
> (Updated Feb. 24, 2016, 9:46 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 allocator metrics for total and allocated scalar resources.
> 
> 
> Diffs
> -
> 
>   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/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
> 
>

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

2016-02-24 Thread Guangya Liu 


> On 二月 24, 2016, 12:26 a.m., Guangya Liu wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 432-444
> > 
> >
> > The allocatd may keeps changing due the resource offer and resource 
> > recovery, I think that this logic should be in `allocate(slaveIds)`?
> 
> Benjamin Bannier wrote:
> We do not read out `slave.allocated` here, we just install a callback 
> doing that. The calculation of the actual value is triggered if the metrics 
> endpoint is hit. Or am I missing an implicit value copy here?

Yes, you are right. So can you please add some comments here to clairfy that 
you install a callback to get the allocated resources?


- Guangya


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


On 二月 24, 2016, 8:46 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43880/
> ---
> 
> (Updated 二月 24, 2016, 8:46 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 allocator metrics for total and allocated scalar resources.
> 
> 
> Diffs
> -
> 
>   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/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
> 
>

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

2016-02-24 Thread Benjamin Bannier 

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

(Updated Feb. 24, 2016, 9:46 a.m.)


Review request for mesos, Alexander Rukletsov and Ben Mahler.


Changes
---

Addressed comments from Guangya Liu.


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


Repository: mesos


Description
---

Added allocator metrics for total and allocated scalar resources.


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

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

2016-02-24 Thread Benjamin Bannier 


> On Feb. 24, 2016, 1:26 a.m., Guangya Liu wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 432-444
> > 
> >
> > The allocatd may keeps changing due the resource offer and resource 
> > recovery, I think that this logic should be in `allocate(slaveIds)`?

We do not read out `slave.allocated` here, we just install a callback doing 
that. The calculation of the actual value is triggered if the metrics endpoint 
is hit. Or am I missing an implicit value copy here?


- Benjamin


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


On Feb. 23, 2016, 6:09 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43880/
> ---
> 
> (Updated Feb. 23, 2016, 6:09 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
> -
> 
>   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/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
> 
>

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

2016-02-23 Thread Guangya Liu 

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




src/master/allocator/mesos/hierarchical.cpp (lines 432 - 444)


The allocatd may keeps changing due the resource offer and resource 
recovery, I think that this logic should be in `allocate(slaveIds)`?



src/tests/hierarchical_allocator_tests.cpp (line 2353)


s/Course/Coarse-grained ?

ditto for the following



src/tests/hierarchical_allocator_tests.cpp (line 2384)


move this above `removeSlave`

ditto for the following


- Guangya Liu


On 二月 23, 2016, 5:09 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43880/
> ---
> 
> (Updated 二月 23, 2016, 5:09 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
> -
> 
>   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/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
> 
>

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

2016-02-23 Thread Benjamin Bannier 

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

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
-

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