You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@mesos.apache.org by Benjamin Bannier <be...@mesosphere.io> on 2016/02/24 14:49:40 UTC

Re: Review Request 43884: Added allocator metrics for used quotas.

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

(Updated Feb. 24, 2016, 2:49 p.m.)


Review request for mesos, Alexander Rukletsov and Ben Mahler.


Changes
-------

Addressed gyliu's comments.


Summary (updated)
-----------------

Added allocator metrics for used quotas.


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


Repository: mesos


Description (updated)
-------

Added allocator metrics for used quotas.


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/43884/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 43884: Added allocator metrics for used quotas.

Posted by Mesos ReviewBot <re...@mesos.apache.org>.

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



Patch looks great!

Reviews applied: [43879, 43880, 43881, 43882, 43883, 43884]

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

- Mesos ReviewBot


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

Posted by Mesos ReviewBot <re...@mesos.apache.org>.

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



Bad patch!

Reviews applied: [43884, 43883, 43882, 43881, 43880, 43879]

Failed command: ./support/apply-review.sh -n -r 43884

Error:
2016-02-25 19:18:03 URL:https://reviews.apache.org/r/43884/diff/raw/ [3997/3997] -> "43884.patch" [1]
error: patch failed: src/master/allocator/mesos/hierarchical.hpp:382
error: src/master/allocator/mesos/hierarchical.hpp: patch does not apply

Full log: https://builds.apache.org/job/mesos-reviewbot/11647/console

- Mesos ReviewBot


On Feb. 25, 2016, 11:01 a.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43884/
> -----------------------------------------------------------
> 
> (Updated Feb. 25, 2016, 11:01 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
> 
> 
> Bugs: MESOS-4723
>     https://issues.apache.org/jira/browse/MESOS-4723
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added allocator metrics for used quotas.
> 
> 
> 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/43884/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 43884: Added allocator metrics for used quotas.

Posted by Mesos ReviewBot <re...@mesos.apache.org>.

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



Bad review!

Reviews applied: [43884, 43883, 43882, 43881, 43880, 43879]

Error:
No reviewers specified. Please find a reviewer by asking on JIRA or the mailing list.

- Mesos ReviewBot


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

Posted by Klaus Ma <kl...@gmail.com>.

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




src/master/allocator/mesos/hierarchical.hpp (line 389)
<https://reviews.apache.org/r/43884/#comment182802>

    Remove it when removeQuota.



src/master/allocator/mesos/hierarchical.cpp (line 1108)
<https://reviews.apache.org/r/43884/#comment182803>

    This's not necessary; all resources in quotaSorter are `nonRevocable()`.


- Klaus Ma


On Feb. 27, 2016, 1 a.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43884/
> -----------------------------------------------------------
> 
> (Updated Feb. 27, 2016, 1 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
> 
> 
> Bugs: MESOS-4723
>     https://issues.apache.org/jira/browse/MESOS-4723
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added allocator metrics for used quotas.
> 
> 
> 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/43884/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 43884: Added allocator metrics for used quotas.

Posted by Benjamin Bannier <be...@mesosphere.io>.


> On March 1, 2016, 1:19 a.m., Alexander Rojas wrote:
> > src/master/allocator/mesos/hierarchical.hpp, line 303
> > <https://reviews.apache.org/r/43884/diff/9/?file=1274396#file1274396line303>
> >
> >     I'm even wondering whether `auto` here is a better idea than aliasing.

Of the two possible and discouraged approaches _type aliases_ and _auto_ I choose a type alias since we value verbosity.


> On March 1, 2016, 1:19 a.m., Alexander Rojas wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 1100-1109
> > <https://reviews.apache.org/r/43884/diff/9/?file=1274397#file1274397line1100>
> >
> >     As in my previous comments, let's shy away from implicit calling a constructor. I  this case I think creating the `Deferred` object outside even makes more sense.

Fixed with specific and named getters like elsewhere.


- Benjamin


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


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/43884/
> -----------------------------------------------------------
> 
> (Updated March 2, 2016, 11:34 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
> 
> 
> Bugs: MESOS-4723
>     https://issues.apache.org/jira/browse/MESOS-4723
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added allocator metrics for used quotas.
> 
> 
> 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/43884/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 43884: Added allocator metrics for used quotas.

Posted by Alexander Rojas <al...@mesosphere.io>.

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




src/master/allocator/mesos/hierarchical.hpp (line 302)
<https://reviews.apache.org/r/43884/#comment183036>

    Let's use `typedef` instead. If you do a search this would be the only instance in the codebase of type aliasing using `using`.



src/master/allocator/mesos/hierarchical.hpp (line 303)
<https://reviews.apache.org/r/43884/#comment183037>

    I'm even wondering whether `auto` here is a better idea than aliasing.



src/master/allocator/mesos/hierarchical.cpp (line 1094)
<https://reviews.apache.org/r/43884/#comment183038>

    Comment in next line.



src/master/allocator/mesos/hierarchical.cpp (lines 1100 - 1109)
<https://reviews.apache.org/r/43884/#comment183039>

    As in my previous comments, let's shy away from implicit calling a constructor. I  this case I think creating the `Deferred` object outside even makes more sense.


- Alexander Rojas


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/43884/
> -----------------------------------------------------------
> 
> (Updated Feb. 29, 2016, 8:59 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
> 
> 
> Bugs: MESOS-4723
>     https://issues.apache.org/jira/browse/MESOS-4723
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added allocator metrics for used quotas.
> 
> 
> 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/43884/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 43884: Added allocator metrics for used quotas.

Posted by Mesos ReviewBot <re...@mesos.apache.org>.

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



Patch looks great!

Reviews applied: [44070, 44071, 44073, 44260, 43879, 43880, 43881, 44261, 43882, 43883, 43884]

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

- Mesos ReviewBot


On March 2, 2016, 3:43 p.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43884/
> -----------------------------------------------------------
> 
> (Updated March 2, 2016, 3:43 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
> 
> 
> Bugs: MESOS-4723
>     https://issues.apache.org/jira/browse/MESOS-4723
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> See summary.
> 
> 
> 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/master/allocator/mesos/metrics.cpp PRE-CREATION 
>   src/tests/hierarchical_allocator_tests.cpp 5f771f02db9bd098f3cd36730cd84bf2f5e87a33 
> 
> Diff: https://reviews.apache.org/r/43884/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 43884: Added allocator metrics for used quotas.

Posted by Benjamin Bannier <be...@mesosphere.io>.


> On March 4, 2016, 2:46 p.m., Alexander Rukletsov wrote:
> > docs/monitoring.md, line 905
> > <https://reviews.apache.org/r/43884/diff/14/?file=1279219#file1279219line905>
> >
> >     In the same vein as `allocator/allocated/KIND`, this should be something like `allocator/quota/ROLE/allocated/KIND`, right?

Done.


> On March 4, 2016, 2:46 p.m., Alexander Rukletsov wrote:
> > docs/monitoring.md, line 908
> > <https://reviews.apache.org/r/43884/diff/14/?file=1279219#file1279219line908>
> >
> >     s/in use/allocated

Done.


> On March 4, 2016, 2:46 p.m., Alexander Rukletsov wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 1081-1092
> > <https://reviews.apache.org/r/43884/diff/14/?file=1279221#file1279221line1081>
> >
> >     Here we will be also adding a gauge for "set" quotas, a candidate for a helper in `Metrics`

Set quotas aren't part of this series as they can currently be obtained from a master endpoint, while quota allocations cannot.


> On March 4, 2016, 2:46 p.m., Alexander Rukletsov wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 1124-1132
> > <https://reviews.apache.org/r/43884/diff/14/?file=1279221#file1279221line1124>
> >
> >     Let's do it in `Metrics` as well.

Done.


> On March 4, 2016, 2:46 p.m., Alexander Rukletsov wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 1820
> > <https://reviews.apache.org/r/43884/diff/14/?file=1279221#file1279221line1820>
> >
> >     Let's be consistent and call it `_quota_allocated`. Same for the local variable.

Done.


> On March 4, 2016, 2:46 p.m., Alexander Rukletsov wrote:
> > src/master/allocator/mesos/metrics.hpp, lines 74-75
> > <https://reviews.apache.org/r/43884/diff/14/?file=1279222#file1279222line74>
> >
> >     let's call it `quota_allocated`.

Done.


> On March 4, 2016, 2:46 p.m., Alexander Rukletsov wrote:
> > src/tests/hierarchical_allocator_tests.cpp, line 2474
> > <https://reviews.apache.org/r/43884/diff/14/?file=1279224#file1279224line2474>
> >
> >     s/famework/framework

Done.


> On March 4, 2016, 2:46 p.m., Alexander Rukletsov wrote:
> > src/master/allocator/mesos/metrics.hpp, line 75
> > <https://reviews.apache.org/r/43884/diff/14/?file=1279222#file1279222line75>
> >
> >     two spaces

I might be wrong, but don't we indent by four spaces, unless we wrap an assignment, in which case we indent by two spaces? There's no assignment here.


- Benjamin


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


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/43884/
> -----------------------------------------------------------
> 
> (Updated March 4, 2016, 5:01 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
> 
> 
> Bugs: MESOS-4723
>     https://issues.apache.org/jira/browse/MESOS-4723
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added allocator metrics for used quotas.
> 
> 
> 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/43884/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 43884: Added allocator metrics for used quotas.

Posted by Alexander Rukletsov <ru...@gmail.com>.


> On March 4, 2016, 1:46 p.m., Alexander Rukletsov wrote:
> > src/master/allocator/mesos/metrics.hpp, line 75
> > <https://reviews.apache.org/r/43884/diff/14/?file=1279222#file1279222line75>
> >
> >     two spaces
> 
> Benjamin Bannier wrote:
>     I might be wrong, but don't we indent by four spaces, unless we wrap an assignment, in which case we indent by two spaces? There's no assignment here.

I believe we indent by two, except when we leave `(` on the previous line.


- Alexander


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


On March 4, 2016, 4:01 p.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43884/
> -----------------------------------------------------------
> 
> (Updated March 4, 2016, 4:01 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
> 
> 
> Bugs: MESOS-4723
>     https://issues.apache.org/jira/browse/MESOS-4723
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added allocator metrics for used quotas.
> 
> 
> 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/43884/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 43884: Added allocator metrics for used quotas.

Posted by Alexander Rukletsov <ru...@gmail.com>.

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




docs/monitoring.md (line 905)
<https://reviews.apache.org/r/43884/#comment183916>

    In the same vein as `allocator/allocated/KIND`, this should be something like `allocator/quota/ROLE/allocated/KIND`, right?



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

    s/in use/allocated



src/master/allocator/mesos/hierarchical.cpp (lines 1081 - 1092)
<https://reviews.apache.org/r/43884/#comment183917>

    Here we will be also adding a gauge for "set" quotas, a candidate for a helper in `Metrics`



src/master/allocator/mesos/hierarchical.cpp (lines 1124 - 1132)
<https://reviews.apache.org/r/43884/#comment183919>

    Let's do it in `Metrics` as well.



src/master/allocator/mesos/hierarchical.cpp (line 1820)
<https://reviews.apache.org/r/43884/#comment183943>

    Let's be consistent and call it `_quota_allocated`. Same for the local variable.



src/master/allocator/mesos/metrics.hpp (lines 74 - 75)
<https://reviews.apache.org/r/43884/#comment183942>

    let's call it `quota_allocated`.



src/master/allocator/mesos/metrics.hpp (line 75)
<https://reviews.apache.org/r/43884/#comment183922>

    two spaces



src/tests/hierarchical_allocator_tests.cpp (line 2474)
<https://reviews.apache.org/r/43884/#comment183923>

    s/famework/framework


- 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/43884/
> -----------------------------------------------------------
> 
> (Updated March 3, 2016, 4:17 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
> 
> 
> Bugs: MESOS-4723
>     https://issues.apache.org/jira/browse/MESOS-4723
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added allocator metrics for used quotas.
> 
> 
> 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/43884/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 43884: Added allocator metrics for used quotas.

Posted by Klaus Ma <kl...@gmail.com>.

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




src/master/allocator/mesos/metrics.hpp (line 98)
<https://reviews.apache.org/r/43884/#comment184146>

    Add comments on keys, e.g. role : <resources_type: Gauge>.


- Klaus Ma


On March 5, 2016, 12:01 a.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43884/
> -----------------------------------------------------------
> 
> (Updated March 5, 2016, 12:01 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
> 
> 
> Bugs: MESOS-4723
>     https://issues.apache.org/jira/browse/MESOS-4723
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added allocator metrics for used quotas.
> 
> 
> 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/43884/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 43884: Added allocator metrics for used quotas.

Posted by Mesos ReviewBot <re...@mesos.apache.org>.

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



Patch looks great!

Reviews applied: [44070, 44071, 44073, 44260, 43879, 43880, 43881, 44261, 43882, 43883, 43884]

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

- Mesos ReviewBot


On March 4, 2016, 4:01 p.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43884/
> -----------------------------------------------------------
> 
> (Updated March 4, 2016, 4:01 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
> 
> 
> Bugs: MESOS-4723
>     https://issues.apache.org/jira/browse/MESOS-4723
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added allocator metrics for used quotas.
> 
> 
> 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/43884/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 43884: Added allocator metrics for used quotas.

Posted by Ben Mahler <be...@gmail.com>.

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


Fix it, then Ship it!




Looks good! I'll make some minor adjustments based on the comments prior to getting this committed.


docs/monitoring.md (lines 876 - 890)
<https://reviews.apache.org/r/43884/#comment187539>

    Hm.. looks like the "resources/" prefix was left out? Any reason for that? The "resources/" prefix helps us if we add any quota metrics that are not tied to the resources.



docs/monitoring.md (line 880)
<https://reviews.apache.org/r/43884/#comment187526>

    "offered or allocated" here as well?



src/master/allocator/mesos/hierarchical.cpp (line 1697)
<https://reviews.apache.org/r/43884/#comment187540>

    Just 2 spaces need here instead of 4



src/master/allocator/mesos/metrics.hpp (lines 58 - 64)
<https://reviews.apache.org/r/43884/#comment187543>

    s/resource kind/resource/ just because we haven't generally been using the work "kind" in this manner to my knowledge
    
    Only 2 spaces needed on the continuation lines here.



src/master/allocator/mesos/metrics.cpp (line 21)
<https://reviews.apache.org/r/43884/#comment187554>

    This should go above process and stout here?



src/master/allocator/mesos/metrics.cpp (line 37)
<https://reviews.apache.org/r/43884/#comment187544>

    How about a .self() here insted of taking a reference?



src/master/allocator/mesos/metrics.cpp (line 92)
<https://reviews.apache.org/r/43884/#comment187551>

    Any reason not to just pass the 'gauge' here rather than doing the lookup?
    
    ```
          allocated.put(resource.name(), gauge);
          process::metrics::add(gauge);
    ```
    
    Ditto below.



src/master/allocator/mesos/metrics.cpp (line 102)
<https://reviews.apache.org/r/43884/#comment187547>

    2 spaces unless wrapping an open parenthesis



src/master/allocator/mesos/metrics.cpp (line 103)
<https://reviews.apache.org/r/43884/#comment187546>

    you can omit the 'allocator' here to use a deferred context where libprocess manages the execution, see here: https://github.com/apache/mesos/tree/0.28.0/3rdparty/libprocess#defer


- Ben Mahler


On March 18, 2016, 4:08 p.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43884/
> -----------------------------------------------------------
> 
> (Updated March 18, 2016, 4:08 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
> 
> 
> Bugs: MESOS-4723
>     https://issues.apache.org/jira/browse/MESOS-4723
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added allocator metrics for used quotas.
> 
> 
> Diffs
> -----
> 
>   docs/monitoring.md 827f7073204fcf8575ca980a5571c8be4f5e4110 
>   src/master/allocator/mesos/hierarchical.hpp 0c622b8569bc79ae4e365a57e463ca5349356713 
>   src/master/allocator/mesos/hierarchical.cpp 70291075c00a9a557529c2562dedcfc6c6c3ec32 
>   src/master/allocator/mesos/metrics.hpp d04e9a11c77b6fb2d522608e3085f81f8a6657ca 
>   src/master/allocator/mesos/metrics.cpp 46dd7f7b3d8d49c59b42e789fe5dcd49f1693ace 
>   src/tests/hierarchical_allocator_tests.cpp 459e02576f6d05abbbcc83ae5cabac5c66e93f05 
> 
> Diff: https://reviews.apache.org/r/43884/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 43884: Added allocator metrics for used quotas.

Posted by Benjamin Bannier <be...@mesosphere.io>.

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

(Updated March 18, 2016, 5:08 p.m.)


Review request for mesos, Alexander Rukletsov and Ben Mahler.


Changes
-------

Review comments from BenM.


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


Repository: mesos


Description
-------

Added allocator metrics for used quotas.


Diffs (updated)
-----

  docs/monitoring.md 827f7073204fcf8575ca980a5571c8be4f5e4110 
  src/master/allocator/mesos/hierarchical.hpp 0c622b8569bc79ae4e365a57e463ca5349356713 
  src/master/allocator/mesos/hierarchical.cpp 70291075c00a9a557529c2562dedcfc6c6c3ec32 
  src/master/allocator/mesos/metrics.hpp d04e9a11c77b6fb2d522608e3085f81f8a6657ca 
  src/master/allocator/mesos/metrics.cpp 46dd7f7b3d8d49c59b42e789fe5dcd49f1693ace 
  src/tests/hierarchical_allocator_tests.cpp 459e02576f6d05abbbcc83ae5cabac5c66e93f05 

Diff: https://reviews.apache.org/r/43884/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 43884: Added allocator metrics for used quotas.

Posted by Ben Mahler <be...@gmail.com>.


> On March 17, 2016, 9:07 p.m., Ben Mahler wrote:
> > docs/monitoring.md, lines 876-883
> > <https://reviews.apache.org/r/43884/diff/16/?file=1299847#file1299847line876>
> >
> >     Thanks! How about:
> >     
> >     ```
> >     <tr>
> >       <td>
> >       <code>allocator/mesos/quota/<role>/<resource>_allocated</code>
> >       </td>
> >       <td>Amount of resources considered allocated towards a role's quota guarantee</td>
> >       <td>Gauge</td>
> >     </tr>
> >     ```
> >     
> >     Also, can you add the guarantee to this patch?
> >     
> >     ```
> >     <tr>
> >       <td>
> >       <code>allocator/mesos/quota/<role>/<resource>_guarantee</code>
> >       </td>
> >       <td>Amount of resources guaranteed for a role via quota</td>
> >       <td>Gauge</td>
> >     </tr>
> >     ```
> >     
> >     Then we can easily monitor and alert on the distance between the two, and you can imagine future additions to enable better visibility into the system, for example:
> >     
> >     ```
> >     allocator/mesos/quota/role/cpus_guarantee
> >     allocator/mesos/quota/role/cpus_allocated
> >     allocator/mesos/quota/role/cpus_filtered
> >     allocator/mesos/quota/role/cpus_unsatisfiable
> >     ```

Sorry one more thought here. After chatting with vinod about our metric naming mistakes we made in the past (e.g. master metrics), we're thinking the following would be better here:

Per-role metrics:
`"allocator/mesos/quota/roles/<role>/resources/<resource>/guarantee"`

Aggregated across-role metrics:
`"allocator/mesos/quota/resources/<resource>/guarantee"`

For dynamically generated metrics, we now prefer using an explicit delimiter (like `"roles/"`) to delimit metric names when there is a potential for additional metrics to be added or aggregated versions are needed. Let's say we want to add a quota metric that isn't tied to resources, we can easily do so. For example, if we added priorities we could easily add `"allocator/mesos/quota/priority/1"` without the potential of `"priority"` colliding with a role (thanks to the `"roles/"` and `"resources"` delimiter. For the aggregated case, clearly we'll have a conflict with my original suggestion above if one were to set quota for a role called `"cpus"`.

Thoughts?


- Ben


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


On March 15, 2016, 2:51 p.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43884/
> -----------------------------------------------------------
> 
> (Updated March 15, 2016, 2:51 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
> 
> 
> Bugs: MESOS-4723
>     https://issues.apache.org/jira/browse/MESOS-4723
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added allocator metrics for used quotas.
> 
> 
> Diffs
> -----
> 
>   docs/monitoring.md 827f7073204fcf8575ca980a5571c8be4f5e4110 
>   src/master/allocator/mesos/hierarchical.hpp 52b3a9bfbe73d24968f7ddb0672aee1e05636ab0 
>   src/master/allocator/mesos/hierarchical.cpp 70291075c00a9a557529c2562dedcfc6c6c3ec32 
>   src/master/allocator/mesos/metrics.hpp d04e9a11c77b6fb2d522608e3085f81f8a6657ca 
>   src/master/allocator/mesos/metrics.cpp 46dd7f7b3d8d49c59b42e789fe5dcd49f1693ace 
>   src/tests/hierarchical_allocator_tests.cpp 459e02576f6d05abbbcc83ae5cabac5c66e93f05 
> 
> Diff: https://reviews.apache.org/r/43884/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 43884: Added allocator metrics for used quotas.

Posted by Ben Mahler <be...@gmail.com>.


> On March 17, 2016, 9:07 p.m., Ben Mahler wrote:
> > docs/monitoring.md, lines 876-883
> > <https://reviews.apache.org/r/43884/diff/16/?file=1299847#file1299847line876>
> >
> >     Thanks! How about:
> >     
> >     ```
> >     <tr>
> >       <td>
> >       <code>allocator/mesos/quota/<role>/<resource>_allocated</code>
> >       </td>
> >       <td>Amount of resources considered allocated towards a role's quota guarantee</td>
> >       <td>Gauge</td>
> >     </tr>
> >     ```
> >     
> >     Also, can you add the guarantee to this patch?
> >     
> >     ```
> >     <tr>
> >       <td>
> >       <code>allocator/mesos/quota/<role>/<resource>_guarantee</code>
> >       </td>
> >       <td>Amount of resources guaranteed for a role via quota</td>
> >       <td>Gauge</td>
> >     </tr>
> >     ```
> >     
> >     Then we can easily monitor and alert on the distance between the two, and you can imagine future additions to enable better visibility into the system, for example:
> >     
> >     ```
> >     allocator/mesos/quota/role/cpus_guarantee
> >     allocator/mesos/quota/role/cpus_allocated
> >     allocator/mesos/quota/role/cpus_filtered
> >     allocator/mesos/quota/role/cpus_unsatisfiable
> >     ```
> 
> Ben Mahler wrote:
>     Sorry one more thought here. After chatting with vinod about our metric naming mistakes we made in the past (e.g. master metrics), we're thinking the following would be better here:
>     
>     Per-role metrics:
>     `"allocator/mesos/quota/roles/<role>/resources/<resource>/guarantee"`
>     
>     Aggregated across-role metrics:
>     `"allocator/mesos/quota/resources/<resource>/guarantee"`
>     
>     For dynamically generated metrics, we now prefer using an explicit delimiter (like `"roles/"`) to delimit metric names when there is a potential for additional metrics to be added or aggregated versions are needed. Let's say we want to add a quota metric that isn't tied to resources, we can easily do so. For example, if we added priorities we could easily add `"allocator/mesos/quota/priority/1"` without the potential of `"priority"` colliding with a role (thanks to the `"roles/"` and `"resources"` delimiter. For the aggregated case, clearly we'll have a conflict with my original suggestion above if one were to set quota for a role called `"cpus"`.
>     
>     Thoughts?

And another thought :)

I'd like to avoid using the word "allocated" here to indicate both offered and allocated, because in the future (when the allocator understands offers), we will want to distinguish the two. After chatting with Vinod about this one, we're thinking the following is the best way to deal with this for now:

`"allocator/mesos/quota/roles/<role>/resources/<resource>/offered_or_allocated"`

Later, we'll remove this in favor of having two seperate metrics:

`"allocator/mesos/quota/roles/<role>/resources/<resource>/allocated"`
`"allocator/mesos/quota/roles/<role>/resources/<resource>/offered"`

If we added `"allocated"` now, splitting the metrics would be a backwards incompatible change because we would be changing the meaning of `"allocated"`.


- Ben


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


On March 15, 2016, 2:51 p.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43884/
> -----------------------------------------------------------
> 
> (Updated March 15, 2016, 2:51 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
> 
> 
> Bugs: MESOS-4723
>     https://issues.apache.org/jira/browse/MESOS-4723
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added allocator metrics for used quotas.
> 
> 
> Diffs
> -----
> 
>   docs/monitoring.md 827f7073204fcf8575ca980a5571c8be4f5e4110 
>   src/master/allocator/mesos/hierarchical.hpp 52b3a9bfbe73d24968f7ddb0672aee1e05636ab0 
>   src/master/allocator/mesos/hierarchical.cpp 70291075c00a9a557529c2562dedcfc6c6c3ec32 
>   src/master/allocator/mesos/metrics.hpp d04e9a11c77b6fb2d522608e3085f81f8a6657ca 
>   src/master/allocator/mesos/metrics.cpp 46dd7f7b3d8d49c59b42e789fe5dcd49f1693ace 
>   src/tests/hierarchical_allocator_tests.cpp 459e02576f6d05abbbcc83ae5cabac5c66e93f05 
> 
> Diff: https://reviews.apache.org/r/43884/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 43884: Added allocator metrics for used quotas.

Posted by Benjamin Bannier <be...@mesosphere.io>.


> On March 17, 2016, 10:07 p.m., Ben Mahler wrote:
> > src/master/allocator/mesos/metrics.cpp, line 73
> > <https://reviews.apache.org/r/43884/diff/16/?file=1299851#file1299851line73>
> >
> >     this is actually a copy since the right hand side is not a temporary, it looks like you only added this to make the gauge name fit in 80 characters?
> >     
> >     I would prefer `resource.name()` to having the extra variable with the same name to save 3 characters, or just s/resourceName/name/. If you create the gauge as a variable you can avoid the need for the variable, and it tends to read easier:
> >     
> >     ```
> >       Gauge gauge = Gauge(
> >           "allocator/mesos/quota/" + role + "/" + resource.name() + "_allocated",
> >           defer(...)
> >       );
> >       
> >       metrics::add(gauge);
> >       
> >       gauges[resource.name()] = gauge;
> >     ```

I introduced this variable so I wouldn't have to capture the whole `Resource` by value for the lambda below. Since I am moving away from using a lambda we can just use `resource.name()` directly.


> On March 17, 2016, 10:07 p.m., Ben Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 1694-1696
> > <https://reviews.apache.org/r/43884/diff/16/?file=1299849#file1299849line1694>
> >
> >     How about the folllowing wrapping:
> >     
> >     ```
> >       Option<Value::Scalar> used =
> >         quotaRoleSorter->allocationScalarQuantities(role)
> >           .get<Value::Scalar>(resource);
> >     ```

Done, I added two more spaces before `.get`.


> On March 17, 2016, 10:07 p.m., Ben Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 1698
> > <https://reviews.apache.org/r/43884/diff/16/?file=1299849#file1299849line1698>
> >
> >     Recall that we have an `->` operator now to avoid the `.get().` pattern:
> >     
> >     ```
> >     return used.isSome() ? used->value() : 0;
> >     ```

Great to hear that this is the pattern we can follow now.


> On March 17, 2016, 10:07 p.m., Ben Mahler wrote:
> > src/master/allocator/mesos/metrics.cpp, lines 59-64
> > <https://reviews.apache.org/r/43884/diff/16/?file=1299851#file1299851line59>
> >
> >     How about avoiding the need for the klunky typedef here:
> >     
> >     ```
> >     foreachkey (const string& role, quota_allocated) {
> >       foreachvalue (const Gauge& gauge, quota_allocated[role]) {
> >         metrics::remove(gauge);
> >       }
> >     }
> >     ```

Done.


> On March 17, 2016, 10:07 p.m., Ben Mahler wrote:
> > src/master/allocator/mesos/metrics.cpp, lines 68-87
> > <https://reviews.apache.org/r/43884/diff/16/?file=1299851#file1299851line68>
> >
> >     Could we encode our assumption that this is not called to change quota, but rather only called to initially specify a quota? In other words, adding a CHECK here that we don't already know about this role in terms of quota. Otherwise, it will silently misbehave as we discussed (leak metrics).

Done.


> On March 17, 2016, 10:07 p.m., Ben Mahler wrote:
> > src/master/allocator/mesos/metrics.cpp, lines 79-81
> > <https://reviews.apache.org/r/43884/diff/16/?file=1299851#file1299851line79>
> >
> >     Why the defer indirection here as opposed to direct defers? Per our offline conversation this was because we made the member functions const? If so, let's make them non-const for now as that's the pattern we've put in place to deal with the defer to const issue.
> >     
> >     Would love to see the defer to const issue fixed, but let's tackle that later :)

I changed the member function signatures enabling us to use the preferred way to defer member function calls. This also allowed me to change `Metrics` to hold a `PID<HierarchicalAllocatorProcess>` simplifying the ownership semantics.


> On March 17, 2016, 10:07 p.m., Ben Mahler wrote:
> > src/master/allocator/mesos/metrics.hpp, lines 47-50
> > <https://reviews.apache.org/r/43884/diff/16/?file=1299850#file1299850line47>
> >
> >     I don't follow why the comment here means that we had to remove the qualifiers.. do you need this? If not, let's take them out for now since we should just think about this as a 'struct' rather than a 'class'.

I felt this was desirable to include since we were holding a raw `HierarchicalAllocatorProcess*`. Since we now hold a `PID<HierarchicalAllocatorProcess>` there is no need anymore to lock the `Metrics` class down.


> On March 17, 2016, 10:07 p.m., Ben Mahler wrote:
> > src/master/allocator/mesos/metrics.hpp, lines 41-42
> > <https://reviews.apache.org/r/43884/diff/16/?file=1299850#file1299850line41>
> >
> >     Actually, could we make `Metrics` a struct and remove the access qualifiers? This is the pattern in place for the master's metrics, these Metrics containers are generally meant to just be a wrapper around the metrics rather than a full fledged abstraction, so we made it a struct and didn't bother with access qualifiers.
> >     
> >     Generally member variables go below member functions, could you move this down?
> >     
> >     Also, any reason you needed the pointer instead of a PID? It looks like we could just defer using the PID here?

Like you wrote below I couldn't `defer` using `PID` since my member functions where `const` an no suitable overload for that exists for `defer`. I made the called member functions non-const allowing us to hold a `PID` directly simplifying the situation.


> On March 17, 2016, 10:07 p.m., Ben Mahler wrote:
> > src/master/allocator/mesos/metrics.hpp, lines 35-38
> > <https://reviews.apache.org/r/43884/diff/16/?file=1299850#file1299850line35>
> >
> >     I don't know, this class is so tightly coupled to the allocator process that I'd rather not have the NOTE (I see this as an embedded struct in the allocator that we've just happened to have pulled up). How about just the following:
> >     
> >     ```
> >     // Collection of metrics for the allocator, these begin with
> >     // the following prefix: "allocator/mesos/".
> >     ```

Done.


> On March 17, 2016, 10:07 p.m., Ben Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 1692
> > <https://reviews.apache.org/r/43884/diff/16/?file=1299849#file1299849line1692>
> >
> >     Can you wrap each argument on its own line? That would be more consistent with the wrapping we usually do for function signatures.

Done.


> On March 17, 2016, 10:07 p.m., Ben Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 1691-1692
> > <https://reviews.apache.org/r/43884/diff/16/?file=1299849#file1299849line1691>
> >
> >     How about s/resourceName/resource/ here? Looking at the master metrics code that seems to be more consistent, and this looks to be the first time we've used it:
> >     
> >     ```
> >     $ grep -R resourceName src
> >     $
> >     ```

Done.


> On March 17, 2016, 10:07 p.m., Ben Mahler wrote:
> > src/master/allocator/mesos/hierarchical.hpp, lines 290-291
> > <https://reviews.apache.org/r/43884/diff/16/?file=1299848#file1299848line290>
> >
> >     Could you wrap the arguments each on their own line to be more consistent with our typical signature wrapping?

Done.


> On March 17, 2016, 10:07 p.m., Ben Mahler wrote:
> > src/master/allocator/mesos/metrics.cpp, line 70
> > <https://reviews.apache.org/r/43884/diff/16/?file=1299851#file1299851line70>
> >
> >     How about s/quotaedResources/gauges/ here? I also find the use of "quotaed" unfortunate (I'm guessing you borrowed this from the quota code) since there isn't a clear difference between `quotaResources` and `quotaedResources` to me.

Done.


> On March 17, 2016, 10:07 p.m., Ben Mahler wrote:
> > src/master/allocator/mesos/metrics.cpp, lines 92-99
> > <https://reviews.apache.org/r/43884/diff/16/?file=1299851#file1299851line92>
> >
> >     How about a CHECK on .contains, then using the [] in the foreachvalue loop? This avoids the need for the extra `roleQuotaGauges` variable:
> >     
> >     ```
> >     CHECK(quota_allocated.contains(role));
> >     
> >     foreachvalue (const Gauge& gauge, quota_allocated[role]) {
> >       process::metrics::remove(gauge);
> >     }
> >     
> >     quota_allocated.erase(role);
> >     ```

Yes, this reads much nicer.


> On March 17, 2016, 10:07 p.m., Ben Mahler wrote:
> > src/master/allocator/mesos/metrics.cpp, line 86
> > <https://reviews.apache.org/r/43884/diff/16/?file=1299851#file1299851line86>
> >
> >     How about using the [] operator? It tends to make the key and value read more easily:
> >     
> >     ```
> >     quota_allocated[role] = gauges;
> >     ```

Note that this syntax is not possible a few lines above since a `Gauge` doesn't have a default ctr, and `put` is the only option there. My original motivation was to use a single style to add entries to the various maps. I am changing this one instance here as you suggested.


> On March 17, 2016, 10:07 p.m., Ben Mahler wrote:
> > src/tests/hierarchical_allocator_tests.cpp, lines 1660-1664
> > <https://reviews.apache.org/r/43884/diff/16/?file=1299852#file1299852line1660>
> >
> >     s/metricKey/metric/ would be more consistent
> >     
> >     Is it possible to use the JSON contains helper here to make the test a bit easier to read? Any reason you avoided checking the values of these?
> >     
> >     ```
> >     JSON::Object expected = {
> >       {"allocator/mesos/quota/" + QUOTA_ROLE + "/cpus_allocated", x},
> >       {"allocator/mesos/quota/" + QUOTA_ROLE + "/mem_allocated", y},
> >     };
> >     
> >     EXPECT_TRUE(metrics.contains(expected));
> >     ```
> >     
> >     Ditto below

I `s/metricKey/metric/g`.

The reason I am not checking the values here is that I explicitly check for entry absence; would you rather have me check obtained values against `JSON::Null`?

To update the code below to use `contains` I'd have to adapt your suggestion. `contains` is a member function of `JSON::Value` while `Metrics` returns a `JSON::Object`, so to use `contains` we need to create a `JSON::Value` somewhere. Since we cannot use `contains` to check for value absence we still need access to the `JSON::Object`'s `values`. If we'd choose to create the temporary just before calling `contains` instead of e.g., using `JSON::Value metrics = Metrics();` so we would not need to `ASSERT` that `metrics` `is<JSON::Object>`, the resulting code would read

    JSON::Object metrics = Metrics();
    JSON::Object expected;
    expected.values = {
        {"allocator/mesos/quota/" + QUOTA_ROLE + "/cpus_allocated", 0.25},
        {"allocator/mesos/quota/" + QUOTA_ROLE + "/mem_allocated", 128}};
    EXPECT_TRUE(JSON::Value(metrics).contains(expected));
    string metric = "allocator/mesos/quota/" + QUOTA_ROLE + "/disk_allocated";
    EXPECT_EQ(0u, metrics.values.count(metric));

as opposed to originally

    JSON::Object metrics = Metrics();
    string metricKey = "allocator/mesos/quota/" + QUOTA_ROLE + "/cpus_allocated";
    EXPECT_EQ(0.25, metrics.values[metricKey]);
    metricKey = "allocator/mesos/quota/" + QUOTA_ROLE + "/mem_allocated";
    EXPECT_EQ(128, metrics.values[metricKey]);
    metricKey = "allocator/mesos/quota/" + QUOTA_ROLE + "/disk_allocated";
    EXPECT_EQ(0u, metrics.values.count(metricKey));

I have the feeling the changed version requires more mental effort to parse.


- Benjamin


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


On March 18, 2016, 5:08 p.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43884/
> -----------------------------------------------------------
> 
> (Updated March 18, 2016, 5:08 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
> 
> 
> Bugs: MESOS-4723
>     https://issues.apache.org/jira/browse/MESOS-4723
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added allocator metrics for used quotas.
> 
> 
> Diffs
> -----
> 
>   docs/monitoring.md 827f7073204fcf8575ca980a5571c8be4f5e4110 
>   src/master/allocator/mesos/hierarchical.hpp 0c622b8569bc79ae4e365a57e463ca5349356713 
>   src/master/allocator/mesos/hierarchical.cpp 70291075c00a9a557529c2562dedcfc6c6c3ec32 
>   src/master/allocator/mesos/metrics.hpp d04e9a11c77b6fb2d522608e3085f81f8a6657ca 
>   src/master/allocator/mesos/metrics.cpp 46dd7f7b3d8d49c59b42e789fe5dcd49f1693ace 
>   src/tests/hierarchical_allocator_tests.cpp 459e02576f6d05abbbcc83ae5cabac5c66e93f05 
> 
> Diff: https://reviews.apache.org/r/43884/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 43884: Added allocator metrics for used quotas.

Posted by Ben Mahler <be...@gmail.com>.

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



Thanks Benjamin! The main higher level suggestion here is to add the guarantees alongside the allocations.


docs/monitoring.md (lines 876 - 883)
<https://reviews.apache.org/r/43884/#comment186445>

    Thanks! How about:
    
    ```
    <tr>
      <td>
      <code>allocator/mesos/quota/<role>/<resource>_allocated</code>
      </td>
      <td>Amount of resources considered allocated towards a role's quota guarantee</td>
      <td>Gauge</td>
    </tr>
    ```
    
    Also, can you add the guarantee to this patch?
    
    ```
    <tr>
      <td>
      <code>allocator/mesos/quota/<role>/<resource>_guarantee</code>
      </td>
      <td>Amount of resources guaranteed for a role via quota</td>
      <td>Gauge</td>
    </tr>
    ```
    
    Then we can easily monitor and alert on the distance between the two, and you can imagine future additions to enable better visibility into the system, for example:
    
    ```
    allocator/mesos/quota/role/cpus_guarantee
    allocator/mesos/quota/role/cpus_allocated
    allocator/mesos/quota/role/cpus_filtered
    allocator/mesos/quota/role/cpus_unsatisfiable
    ```



src/master/allocator/mesos/hierarchical.hpp (lines 290 - 291)
<https://reviews.apache.org/r/43884/#comment186436>

    Could you wrap the arguments each on their own line to be more consistent with our typical signature wrapping?



src/master/allocator/mesos/hierarchical.cpp (lines 1691 - 1692)
<https://reviews.apache.org/r/43884/#comment186431>

    How about s/resourceName/resource/ here? Looking at the master metrics code that seems to be more consistent, and this looks to be the first time we've used it:
    
    ```
    $ grep -R resourceName src
    $
    ```



src/master/allocator/mesos/hierarchical.cpp (line 1692)
<https://reviews.apache.org/r/43884/#comment186432>

    Can you wrap each argument on its own line? That would be more consistent with the wrapping we usually do for function signatures.



src/master/allocator/mesos/hierarchical.cpp (lines 1694 - 1696)
<https://reviews.apache.org/r/43884/#comment186433>

    How about the folllowing wrapping:
    
    ```
      Option<Value::Scalar> used =
        quotaRoleSorter->allocationScalarQuantities(role)
          .get<Value::Scalar>(resource);
    ```



src/master/allocator/mesos/hierarchical.cpp (line 1698)
<https://reviews.apache.org/r/43884/#comment186430>

    Recall that we have an `->` operator now to avoid the `.get().` pattern:
    
    ```
    return used.isSome() ? used->value() : 0;
    ```



src/master/allocator/mesos/metrics.hpp (lines 35 - 38)
<https://reviews.apache.org/r/43884/#comment186429>

    I don't know, this class is so tightly coupled to the allocator process that I'd rather not have the NOTE (I see this as an embedded struct in the allocator that we've just happened to have pulled up). How about just the following:
    
    ```
    // Collection of metrics for the allocator, these begin with
    // the following prefix: "allocator/mesos/".
    ```



src/master/allocator/mesos/metrics.hpp (lines 41 - 42)
<https://reviews.apache.org/r/43884/#comment186427>

    Actually, could we make `Metrics` a struct and remove the access qualifiers? This is the pattern in place for the master's metrics, these Metrics containers are generally meant to just be a wrapper around the metrics rather than a full fledged abstraction, so we made it a struct and didn't bother with access qualifiers.
    
    Generally member variables go below member functions, could you move this down?
    
    Also, any reason you needed the pointer instead of a PID? It looks like we could just defer using the PID here?



src/master/allocator/mesos/metrics.hpp (lines 47 - 50)
<https://reviews.apache.org/r/43884/#comment186428>

    I don't follow why the comment here means that we had to remove the qualifiers.. do you need this? If not, let's take them out for now since we should just think about this as a 'struct' rather than a 'class'.



src/master/allocator/mesos/metrics.cpp (lines 59 - 64)
<https://reviews.apache.org/r/43884/#comment186412>

    How about avoiding the need for the klunky typedef here:
    
    ```
    foreachkey (const string& role, quota_allocated) {
      foreachvalue (const Gauge& gauge, quota_allocated[role]) {
        metrics::remove(gauge);
      }
    }
    ```



src/master/allocator/mesos/metrics.cpp (lines 68 - 87)
<https://reviews.apache.org/r/43884/#comment186407>

    Could we encode our assumption that this is not called to change quota, but rather only called to initially specify a quota? In other words, adding a CHECK here that we don't already know about this role in terms of quota. Otherwise, it will silently misbehave as we discussed (leak metrics).



src/master/allocator/mesos/metrics.cpp (line 70)
<https://reviews.apache.org/r/43884/#comment186409>

    How about s/quotaedResources/gauges/ here? I also find the use of "quotaed" unfortunate (I'm guessing you borrowed this from the quota code) since there isn't a clear difference between `quotaResources` and `quotaedResources` to me.



src/master/allocator/mesos/metrics.cpp (line 73)
<https://reviews.apache.org/r/43884/#comment186408>

    this is actually a copy since the right hand side is not a temporary, it looks like you only added this to make the gauge name fit in 80 characters?
    
    I would prefer `resource.name()` to having the extra variable with the same name to save 3 characters, or just s/resourceName/name/. If you create the gauge as a variable you can avoid the need for the variable, and it tends to read easier:
    
    ```
      Gauge gauge = Gauge(
          "allocator/mesos/quota/" + role + "/" + resource.name() + "_allocated",
          defer(...)
      );
      
      metrics::add(gauge);
      
      gauges[resource.name()] = gauge;
    ```



src/master/allocator/mesos/metrics.cpp (lines 79 - 81)
<https://reviews.apache.org/r/43884/#comment186426>

    Why the defer indirection here as opposed to direct defers? Per our offline conversation this was because we made the member functions const? If so, let's make them non-const for now as that's the pattern we've put in place to deal with the defer to const issue.
    
    Would love to see the defer to const issue fixed, but let's tackle that later :)



src/master/allocator/mesos/metrics.cpp (line 86)
<https://reviews.apache.org/r/43884/#comment186410>

    How about using the [] operator? It tends to make the key and value read more easily:
    
    ```
    quota_allocated[role] = gauges;
    ```



src/master/allocator/mesos/metrics.cpp (lines 92 - 99)
<https://reviews.apache.org/r/43884/#comment186411>

    How about a CHECK on .contains, then using the [] in the foreachvalue loop? This avoids the need for the extra `roleQuotaGauges` variable:
    
    ```
    CHECK(quota_allocated.contains(role));
    
    foreachvalue (const Gauge& gauge, quota_allocated[role]) {
      process::metrics::remove(gauge);
    }
    
    quota_allocated.erase(role);
    ```



src/tests/hierarchical_allocator_tests.cpp (lines 1660 - 1664)
<https://reviews.apache.org/r/43884/#comment186406>

    s/metricKey/metric/ would be more consistent
    
    Is it possible to use the JSON contains helper here to make the test a bit easier to read? Any reason you avoided checking the values of these?
    
    ```
    JSON::Object expected = {
      {"allocator/mesos/quota/" + QUOTA_ROLE + "/cpus_allocated", x},
      {"allocator/mesos/quota/" + QUOTA_ROLE + "/mem_allocated", y},
    };
    
    EXPECT_TRUE(metrics.contains(expected));
    ```
    
    Ditto below


- Ben Mahler


On March 15, 2016, 2:51 p.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43884/
> -----------------------------------------------------------
> 
> (Updated March 15, 2016, 2:51 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
> 
> 
> Bugs: MESOS-4723
>     https://issues.apache.org/jira/browse/MESOS-4723
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added allocator metrics for used quotas.
> 
> 
> Diffs
> -----
> 
>   docs/monitoring.md 827f7073204fcf8575ca980a5571c8be4f5e4110 
>   src/master/allocator/mesos/hierarchical.hpp 52b3a9bfbe73d24968f7ddb0672aee1e05636ab0 
>   src/master/allocator/mesos/hierarchical.cpp 70291075c00a9a557529c2562dedcfc6c6c3ec32 
>   src/master/allocator/mesos/metrics.hpp d04e9a11c77b6fb2d522608e3085f81f8a6657ca 
>   src/master/allocator/mesos/metrics.cpp 46dd7f7b3d8d49c59b42e789fe5dcd49f1693ace 
>   src/tests/hierarchical_allocator_tests.cpp 459e02576f6d05abbbcc83ae5cabac5c66e93f05 
> 
> Diff: https://reviews.apache.org/r/43884/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 43884: Added allocator metrics for used quotas.

Posted by Benjamin Bannier <be...@mesosphere.io>.

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

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


Review request for mesos, Alexander Rukletsov and Ben Mahler.


Changes
-------

Addressed offline comments from Ben Mahler.


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


Repository: mesos


Description
-------

Added allocator metrics for used quotas.


Diffs (updated)
-----

  docs/monitoring.md 827f7073204fcf8575ca980a5571c8be4f5e4110 
  src/master/allocator/mesos/hierarchical.hpp 52b3a9bfbe73d24968f7ddb0672aee1e05636ab0 
  src/master/allocator/mesos/hierarchical.cpp 70291075c00a9a557529c2562dedcfc6c6c3ec32 
  src/master/allocator/mesos/metrics.hpp d04e9a11c77b6fb2d522608e3085f81f8a6657ca 
  src/master/allocator/mesos/metrics.cpp 46dd7f7b3d8d49c59b42e789fe5dcd49f1693ace 
  src/tests/hierarchical_allocator_tests.cpp 459e02576f6d05abbbcc83ae5cabac5c66e93f05 

Diff: https://reviews.apache.org/r/43884/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 43884: Added allocator metrics for used quotas.

Posted by Benjamin Bannier <be...@mesosphere.io>.

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

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


Review request for mesos, Alexander Rukletsov and Ben Mahler.


Changes
-------

Adressed comments from Alex.


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


Repository: mesos


Description
-------

Added allocator metrics for used quotas.


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/43884/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 43884: Added allocator metrics for used quotas.

Posted by Benjamin Bannier <be...@mesosphere.io>.

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

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


Review request for mesos, Alexander Rukletsov and Ben Mahler.


Changes
-------

Addressed comments form Alex.


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


Repository: mesos


Description (updated)
-------

Added allocator metrics for used quotas.


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/43884/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 43884: Added allocator metrics for used quotas.

Posted by Benjamin Bannier <be...@mesosphere.io>.

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

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


Review request for mesos, Alexander Rukletsov and Ben Mahler.


Changes
-------

Rebased for changes in `master`.


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


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/43884/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 43884: Added allocator metrics for used quotas.

Posted by Benjamin Bannier <be...@mesosphere.io>.

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

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


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/43884/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 43884: Added allocator metrics for used quotas.

Posted by Benjamin Bannier <be...@mesosphere.io>.

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

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


Review request for mesos, Alexander Rukletsov and Ben Mahler.


Changes
-------

Backtickified.


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


Repository: mesos


Description
-------

Added allocator metrics for used quotas.


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/43884/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 43884: Added allocator metrics for used quotas.

Posted by Benjamin Bannier <be...@mesosphere.io>.

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

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


Review request for mesos, Alexander Rukletsov and Ben Mahler.


Changes
-------

Added removal of metrics of removed quotas; style fixes.


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


Repository: mesos


Description
-------

Added allocator metrics for used quotas.


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/43884/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 43884: Added allocator metrics for used quotas.

Posted by Mesos ReviewBot <re...@mesos.apache.org>.

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



Bad review!

Reviews applied: [43884, 43883, 43882, 43881, 43880, 43879]

Error:
No reviewers specified. Please find a reviewer by asking on JIRA or the mailing list.

- Mesos ReviewBot


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/43884/
> -----------------------------------------------------------
> 
> (Updated Feb. 29, 2016, 7:59 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
> 
> 
> Bugs: MESOS-4723
>     https://issues.apache.org/jira/browse/MESOS-4723
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added allocator metrics for used quotas.
> 
> 
> 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/43884/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 43884: Added allocator metrics for used quotas.

Posted by Benjamin Bannier <be...@mesosphere.io>.

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

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


Review request for mesos, Alexander Rukletsov and Ben Mahler.


Changes
-------

Rebased.


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


Repository: mesos


Description
-------

Added allocator metrics for used quotas.


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/43884/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 43884: Added allocator metrics for used quotas.

Posted by Mesos ReviewBot <re...@mesos.apache.org>.

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



Bad review!

Reviews applied: [43884, 43883, 43882, 43881, 43880, 43879]

Error:
No reviewers specified. Please find a reviewer by asking on JIRA or the mailing list.

- Mesos ReviewBot


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

Posted by Benjamin Bannier <be...@mesosphere.io>.

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

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


Review request for mesos, Alexander Rukletsov and Ben Mahler.


Changes
-------

Rebased.


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


Repository: mesos


Description
-------

Added allocator metrics for used quotas.


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/43884/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 43884: Added allocator metrics for used quotas.

Posted by Benjamin Bannier <be...@mesosphere.io>.

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

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


Review request for mesos, Alexander Rukletsov and Ben Mahler.


Changes
-------

Rebased and get metrics without a helper.


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


Repository: mesos


Description
-------

Added allocator metrics for used quotas.


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/43884/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 43884: Added allocator metrics for used quotas.

Posted by Benjamin Bannier <be...@mesosphere.io>.

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

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


Review request for mesos, Alexander Rukletsov and Ben Mahler.


Changes
-------

Added documentation, fixed ws, and stayed away from C++ range-based `for` loops.


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


Repository: mesos


Description
-------

Added allocator metrics for used quotas.


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