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/23 18:09:08 UTC
Review Request 43881: Added allocator metric for the number of
allocations to a framework.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43881/
-----------------------------------------------------------
Review request for mesos, Alexander Rukletsov and Ben Mahler.
Bugs: MESOS-4719
https://issues.apache.org/jira/browse/MESOS-4719
Repository: mesos
Description
-------
Added allocator metric for the number of allocations to a framework.
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/43881/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 43881: Added allocator metric for the number of
allocations to a framework.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On Feb. 24, 2016, 1:53 a.m., Guangya Liu wrote:
> > src/tests/hierarchical_allocator_tests.cpp, lines 2369-2370
> > <https://reviews.apache.org/r/43881/diff/1/?file=1265551#file1265551line2369>
> >
> > What about following format?
> >
> > allocator->setQuota(
> > framework1.role(),
> > createQuota(framework1.role(),
> > "disk:10"));
I think this fits on a single line well, so let's be a little conservative with making this function even longer.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43881/#review120422
-----------------------------------------------------------
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/43881/
> -----------------------------------------------------------
>
> (Updated Feb. 23, 2016, 6:09 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
>
>
> Bugs: MESOS-4719
> https://issues.apache.org/jira/browse/MESOS-4719
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added allocator metric for the number of allocations to a framework.
>
>
> 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/43881/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 43881: Added allocator metric for the number of
allocations to a framework.
Posted by Guangya Liu <gy...@gmail.com>.
> On 二月 24, 2016, 12:53 a.m., Guangya Liu wrote:
> > src/tests/hierarchical_allocator_tests.cpp, lines 2369-2370
> > <https://reviews.apache.org/r/43881/diff/1/?file=1265551#file1265551line2369>
> >
> > What about following format?
> >
> > allocator->setQuota(
> > framework1.role(),
> > createQuota(framework1.role(),
> > "disk:10"));
>
> Benjamin Bannier wrote:
> I think this fits on a single line well, so let's be a little conservative with making this function even longer.
yes, then I think that here need 4 spaces
- Guangya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43881/#review120422
-----------------------------------------------------------
On 二月 24, 2016, 9:18 a.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43881/
> -----------------------------------------------------------
>
> (Updated 二月 24, 2016, 9:18 a.m.)
>
>
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
>
>
> Bugs: MESOS-4719
> https://issues.apache.org/jira/browse/MESOS-4719
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added allocator metric for the number of allocations to a framework.
>
>
> 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/43881/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 43881: Added allocator metric for the number of
allocations to a framework.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On Feb. 24, 2016, 1:53 a.m., Guangya Liu wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 1354
> > <https://reviews.apache.org/r/43881/diff/1/?file=1265550#file1265550line1354>
> >
> > The counter may count twice for some cases, one framework may get resource allocation in both allocation stage1(Quota) and stage2(wDRF).
Good point! The way I counted here we wouldn't get the total number of allocations over the cluster, but instead the summed number of allocations over slaves (i.e., allocating on two slaves would have been two allocations instead of one). I fixed the logic for that.
I cannot see how we would currently allocate one the same slave twice (once towards quota, once towards fair share) since we quota also sets a (coarse-grained) max; if such a scenario existed or will exist it should be captured with the current implementation which counts allocations over the cluster.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43881/#review120422
-----------------------------------------------------------
On Feb. 24, 2016, 11:22 a.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43881/
> -----------------------------------------------------------
>
> (Updated Feb. 24, 2016, 11:22 a.m.)
>
>
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
>
>
> Bugs: MESOS-4719
> https://issues.apache.org/jira/browse/MESOS-4719
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added allocator metric for the number of allocations to a framework.
>
>
> 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/43881/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 43881: Added allocator metric for the number of
allocations to a framework.
Posted by Guangya Liu <gy...@gmail.com>.
> On 二月 24, 2016, 12:53 a.m., Guangya Liu wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 1354
> > <https://reviews.apache.org/r/43881/diff/1/?file=1265550#file1265550line1354>
> >
> > The counter may count twice for some cases, one framework may get resource allocation in both allocation stage1(Quota) and stage2(wDRF).
>
> Benjamin Bannier wrote:
> Good point! The way I counted here we wouldn't get the total number of allocations over the cluster, but instead the summed number of allocations over slaves (i.e., allocating on two slaves would have been two allocations instead of one). I fixed the logic for that.
>
> I cannot see how we would currently allocate one the same slave twice (once towards quota, once towards fair share) since we quota also sets a (coarse-grained) max; if such a scenario existed or will exist it should be captured with the current implementation which counts allocations over the cluster.
There can be such case that one agent be allocated in stage1(wDRF) and stage2(wDRF), stage1 only allocate non revocable and reserved resources for an agent, stage2 may allocate revocable resources on the same agent.
I think that your latest patch is good enough because it only count the allocations when allocator is sending out allocaitons.
- Guangya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43881/#review120422
-----------------------------------------------------------
On 二月 24, 2016, 10:49 a.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43881/
> -----------------------------------------------------------
>
> (Updated 二月 24, 2016, 10:49 a.m.)
>
>
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
>
>
> Bugs: MESOS-4719
> https://issues.apache.org/jira/browse/MESOS-4719
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added allocator metric for the number of allocations to a framework.
>
>
> 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/43881/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 43881: Added allocator metric for the number of
allocations to a framework.
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43881/#review120422
-----------------------------------------------------------
src/master/allocator/mesos/hierarchical.cpp (line 1197)
<https://reviews.apache.org/r/43881/#comment181863>
The allocator do not know what is offer, does it make sense to rename this metrics to `allocator/framework_allocations`
src/master/allocator/mesos/hierarchical.cpp (line 1354)
<https://reviews.apache.org/r/43881/#comment181856>
The counter may count twice for some cases, one framework may get resource allocation in both allocation stage1(Quota) and stage2(wDRF).
src/tests/hierarchical_allocator_tests.cpp (lines 2359 - 2360)
<https://reviews.apache.org/r/43881/#comment181861>
4 spaces
src/tests/hierarchical_allocator_tests.cpp (lines 2369 - 2370)
<https://reviews.apache.org/r/43881/#comment181857>
What about following format?
allocator->setQuota(
framework1.role(),
createQuota(framework1.role(),
"disk:10"));
src/tests/hierarchical_allocator_tests.cpp (line 2371)
<https://reviews.apache.org/r/43881/#comment181862>
s/Setting/Setting a
src/tests/hierarchical_allocator_tests.cpp (lines 2390 - 2395)
<https://reviews.apache.org/r/43881/#comment181859>
4 spaces
src/tests/hierarchical_allocator_tests.cpp (lines 2400 - 2403)
<https://reviews.apache.org/r/43881/#comment181858>
4 spaces
src/tests/hierarchical_allocator_tests.cpp (lines 2416 - 2418)
<https://reviews.apache.org/r/43881/#comment181860>
4 spaces
- 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/43881/
> -----------------------------------------------------------
>
> (Updated 二月 23, 2016, 5:09 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
>
>
> Bugs: MESOS-4719
> https://issues.apache.org/jira/browse/MESOS-4719
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added allocator metric for the number of allocations to a framework.
>
>
> 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/43881/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 43881: Added allocator metric for the number of
allocations to a framework.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On Feb. 29, 2016, 6:08 p.m., Alexander Rojas wrote:
> > src/tests/hierarchical_allocator_tests.cpp, lines 2498-2502
> > <https://reviews.apache.org/r/43881/diff/9/?file=1273640#file1273640line2498>
> >
> > This is my fault, I don't know how I missed this, but here you can even reused the last value `metricKey` here.
Missed that one as well. I choose to recalc `metricKey` here to keep the value locally visible.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43881/#review121254
-----------------------------------------------------------
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/43881/
> -----------------------------------------------------------
>
> (Updated Feb. 29, 2016, 8:59 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
>
>
> Bugs: MESOS-4719
> https://issues.apache.org/jira/browse/MESOS-4719
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added allocator metric for the number of allocations to a framework.
>
>
> 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/43881/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 43881: Added allocator metric for the number of
allocations to a framework.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On Feb. 29, 2016, 5:08 p.m., Alexander Rojas wrote:
> > src/tests/hierarchical_allocator_tests.cpp, lines 2498-2502
> > <https://reviews.apache.org/r/43881/diff/9/?file=1273640#file1273640line2498>
> >
> > This is my fault, I don't know how I missed this, but here you can even reused the last value `metricKey` here.
>
> Benjamin Bannier wrote:
> Missed that one as well. I choose to recalc `metricKey` here to keep the value locally visible.
Agree with Alexander, let's be consistent: either use `metricKey` everywhere, or in-place keys with some readable formatting. I'm fine with both : ).
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43881/#review121254
-----------------------------------------------------------
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/43881/
> -----------------------------------------------------------
>
> (Updated Feb. 29, 2016, 7:59 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
>
>
> Bugs: MESOS-4719
> https://issues.apache.org/jira/browse/MESOS-4719
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added allocator metric for the number of allocations to a framework.
>
>
> 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/43881/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 43881: Added allocator metric for the number of
allocations to a framework.
Posted by Alexander Rojas <al...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43881/#review121254
-----------------------------------------------------------
src/tests/hierarchical_allocator_tests.cpp (lines 2498 - 2502)
<https://reviews.apache.org/r/43881/#comment182934>
This is my fault, I don't know how I missed this, but here you can even reused the last value `metricKey` here.
- 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/43881/
> -----------------------------------------------------------
>
> (Updated Feb. 28, 2016, 10:28 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
>
>
> Bugs: MESOS-4719
> https://issues.apache.org/jira/browse/MESOS-4719
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added allocator metric for the number of allocations to a framework.
>
>
> 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/43881/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 43881: Added allocator metric for the number of
allocations to a framework.
Posted by Alexander Rojas <al...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43881/#review121343
-----------------------------------------------------------
src/tests/hierarchical_allocator_tests.cpp (line 2481)
<https://reviews.apache.org/r/43881/#comment183025>
s/{}/None()/
- 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/43881/
> -----------------------------------------------------------
>
> (Updated Feb. 29, 2016, 8:59 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
>
>
> Bugs: MESOS-4719
> https://issues.apache.org/jira/browse/MESOS-4719
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added allocator metric for the number of allocations to a framework.
>
>
> 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/43881/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 43881: Added allocator metric for the number of
allocations to a framework.
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43881/#review121434
-----------------------------------------------------------
src/tests/hierarchical_allocator_tests.cpp (lines 2449 - 2455)
<https://reviews.apache.org/r/43881/#comment183131>
no difference
src/tests/hierarchical_allocator_tests.cpp (line 2463)
<https://reviews.apache.org/r/43881/#comment183133>
s/agent2/`agent2`
s/framework1/`framework1`
src/tests/hierarchical_allocator_tests.cpp (line 2464)
<https://reviews.apache.org/r/43881/#comment183134>
s/. Framework2 /, `framework2`
src/tests/hierarchical_allocator_tests.cpp (line 2496)
<https://reviews.apache.org/r/43881/#comment183135>
s/agent2/`agent2`
s/framework2/`framework2`
src/tests/hierarchical_allocator_tests.cpp (line 2499)
<https://reviews.apache.org/r/43881/#comment183129>
two spaces
- Guangya Liu
On 二月 29, 2016, 7:59 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43881/
> -----------------------------------------------------------
>
> (Updated 二月 29, 2016, 7:59 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
>
>
> Bugs: MESOS-4719
> https://issues.apache.org/jira/browse/MESOS-4719
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added allocator metric for the number of allocations to a framework.
>
>
> 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/43881/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 43881: Added allocator metric for the number of
allocations to a framework.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43881/
-----------------------------------------------------------
(Updated March 21, 2016, 12:08 p.m.)
Review request for mesos, Alexander Rukletsov and Ben Mahler.
Changes
-------
Review comments from @bmahler.
Bugs: MESOS-4719
https://issues.apache.org/jira/browse/MESOS-4719
Repository: mesos
Description
-------
Added allocator metric for the number of allocations to a framework.
Diffs (updated)
-----
docs/monitoring.md 827f7073204fcf8575ca980a5571c8be4f5e4110
src/master/allocator/mesos/hierarchical.cpp 70291075c00a9a557529c2562dedcfc6c6c3ec32
src/master/allocator/mesos/metrics.hpp d04e9a11c77b6fb2d522608e3085f81f8a6657ca
src/master/allocator/mesos/metrics.cpp 46dd7f7b3d8d49c59b42e789fe5dcd49f1693ace
src/tests/hierarchical_allocator_tests.cpp 459e02576f6d05abbbcc83ae5cabac5c66e93f05
Diff: https://reviews.apache.org/r/43881/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 43881: Added allocator metric for the number of
allocations to a framework.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43881/
-----------------------------------------------------------
(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-4719
https://issues.apache.org/jira/browse/MESOS-4719
Repository: mesos
Description (updated)
-------
Added allocator metric for the number of allocations to a framework.
Diffs (updated)
-----
docs/monitoring.md 827f7073204fcf8575ca980a5571c8be4f5e4110
src/master/allocator/mesos/hierarchical.cpp 70291075c00a9a557529c2562dedcfc6c6c3ec32
src/master/allocator/mesos/metrics.hpp d04e9a11c77b6fb2d522608e3085f81f8a6657ca
src/master/allocator/mesos/metrics.cpp 46dd7f7b3d8d49c59b42e789fe5dcd49f1693ace
src/tests/hierarchical_allocator_tests.cpp 459e02576f6d05abbbcc83ae5cabac5c66e93f05
Diff: https://reviews.apache.org/r/43881/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 43881: Added allocator metric for the number of
allocations to a framework.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43881/
-----------------------------------------------------------
(Updated March 4, 2016, 5:01 p.m.)
Review request for mesos, Alexander Rukletsov and Ben Mahler.
Changes
-------
Adressed comments from Alex.
Bugs: MESOS-4719
https://issues.apache.org/jira/browse/MESOS-4719
Repository: mesos
Description (updated)
-------
See summary.
Diffs (updated)
-----
docs/monitoring.md 827f7073204fcf8575ca980a5571c8be4f5e4110
src/master/allocator/mesos/hierarchical.cpp 70291075c00a9a557529c2562dedcfc6c6c3ec32
src/master/allocator/mesos/metrics.hpp PRE-CREATION
src/master/allocator/mesos/metrics.cpp PRE-CREATION
src/tests/hierarchical_allocator_tests.cpp 3e4ad31925e1b815a74d67fa3962d23fa5bc89d1
Diff: https://reviews.apache.org/r/43881/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 43881: Added allocator metric for the number of
allocations to a framework.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On March 4, 2016, 2:05 p.m., Alexander Rukletsov wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 275-280
> > <https://reviews.apache.org/r/43881/diff/15/?file=1279203#file1279203line275>
> >
> > I would pull this into a helper in `Metrics` similar to `createGaugesForResource()`. This way we keep the allocator code minimal.
Done.
> On March 4, 2016, 2:05 p.m., Alexander Rukletsov wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 344-347
> > <https://reviews.apache.org/r/43881/diff/15/?file=1279203#file1279203line344>
> >
> > Maybe here a helper as well.
Done.
> On March 4, 2016, 2:05 p.m., Alexander Rukletsov wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 1498-1499
> > <https://reviews.apache.org/r/43881/diff/15/?file=1279203#file1279203line1498>
> >
> > I think this should be around `allocated()` calls for sorters. When `allocated()` is called, the sorter's internal allocation counter is incremented. I'd say we should keep the counters in sync.
> >
> > Alternatively, we can expose the allocations counter from sorters and query it directly.
For now I put these in the locations where a `Sorter`'s `allocated` is called.
> On March 4, 2016, 2:05 p.m., Alexander Rukletsov wrote:
> > src/master/allocator/mesos/metrics.cpp, lines 59-60
> > <https://reviews.apache.org/r/43881/diff/15/?file=1279205#file1279205line59>
> >
> > If you say `using process::metrics::Counter` at the top, you can fit this into one line : ).
Done, also for other `Metric`s in introduced here in this series.
> On March 4, 2016, 2:05 p.m., Alexander Rukletsov wrote:
> > src/tests/hierarchical_allocator_tests.cpp, line 2464
> > <https://reviews.apache.org/r/43881/diff/15/?file=1279206#file1279206line2464>
> >
> > Backtick `framework1` please.
Done.
> On March 4, 2016, 2:05 p.m., Alexander Rukletsov wrote:
> > src/tests/hierarchical_allocator_tests.cpp, lines 2464-2472
> > <https://reviews.apache.org/r/43881/diff/15/?file=1279206#file1279206line2464>
> >
> > Do you want to set quota as part of your test? Because otherwise we can simplify the test a bit by not setting it and having `agent2` allocated to `framework2`.
It does not hurt introducing it here, and we will make more explicit use of this information at a later point in this series.
> On March 4, 2016, 2:05 p.m., Alexander Rukletsov wrote:
> > src/tests/hierarchical_allocator_tests.cpp, line 2487
> > <https://reviews.apache.org/r/43881/diff/15/?file=1279206#file1279206line2487>
> >
> > Feel free to kill this line
It's dead, Alex :D
> On March 4, 2016, 2:05 p.m., Alexander Rukletsov wrote:
> > src/tests/hierarchical_allocator_tests.cpp, lines 2492-2495
> > <https://reviews.apache.org/r/43881/diff/15/?file=1279206#file1279206line2492>
> >
> > How about
> > ```
> > Clock::advance(flags.allocation_interval);
> > Clock::settle();
> > ++allocations;
> > ```
> > to keep clock manipulation together?
So be it.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43881/#review122053
-----------------------------------------------------------
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/43881/
> -----------------------------------------------------------
>
> (Updated March 4, 2016, 5:01 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
>
>
> Bugs: MESOS-4719
> https://issues.apache.org/jira/browse/MESOS-4719
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> docs/monitoring.md 827f7073204fcf8575ca980a5571c8be4f5e4110
> src/master/allocator/mesos/hierarchical.cpp 70291075c00a9a557529c2562dedcfc6c6c3ec32
> src/master/allocator/mesos/metrics.hpp PRE-CREATION
> src/master/allocator/mesos/metrics.cpp PRE-CREATION
> src/tests/hierarchical_allocator_tests.cpp 3e4ad31925e1b815a74d67fa3962d23fa5bc89d1
>
> Diff: https://reviews.apache.org/r/43881/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 43881: Added allocator metric for the number of
allocations to a framework.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43881/#review122053
-----------------------------------------------------------
Fix it, then Ship it!
src/master/allocator/mesos/hierarchical.cpp (lines 275 - 280)
<https://reviews.apache.org/r/43881/#comment183900>
I would pull this into a helper in `Metrics` similar to `createGaugesForResource()`. This way we keep the allocator code minimal.
src/master/allocator/mesos/hierarchical.cpp (lines 344 - 347)
<https://reviews.apache.org/r/43881/#comment183901>
Maybe here a helper as well.
src/master/allocator/mesos/hierarchical.cpp (lines 1498 - 1499)
<https://reviews.apache.org/r/43881/#comment183903>
I think this should be around `allocated()` calls for sorters. When `allocated()` is called, the sorter's internal allocation counter is incremented. I'd say we should keep the counters in sync.
Alternatively, we can expose the allocations counter from sorters and query it directly.
src/master/allocator/mesos/metrics.cpp (lines 59 - 60)
<https://reviews.apache.org/r/43881/#comment183902>
If you say `using process::metrics::Counter` at the top, you can fit this into one line : ).
src/tests/hierarchical_allocator_tests.cpp (line 2464)
<https://reviews.apache.org/r/43881/#comment183904>
Backtick `framework1` please.
src/tests/hierarchical_allocator_tests.cpp (lines 2464 - 2472)
<https://reviews.apache.org/r/43881/#comment183905>
Do you want to set quota as part of your test? Because otherwise we can simplify the test a bit by not setting it and having `agent2` allocated to `framework2`.
src/tests/hierarchical_allocator_tests.cpp (line 2487)
<https://reviews.apache.org/r/43881/#comment183907>
Feel free to kill this line
src/tests/hierarchical_allocator_tests.cpp (lines 2492 - 2495)
<https://reviews.apache.org/r/43881/#comment183906>
How about
```
Clock::advance(flags.allocation_interval);
Clock::settle();
++allocations;
```
to keep clock manipulation together?
- 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/43881/
> -----------------------------------------------------------
>
> (Updated March 3, 2016, 4:17 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
>
>
> Bugs: MESOS-4719
> https://issues.apache.org/jira/browse/MESOS-4719
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added allocator metric for the number of allocations to a framework.
>
>
> Diffs
> -----
>
> docs/monitoring.md 827f7073204fcf8575ca980a5571c8be4f5e4110
> src/master/allocator/mesos/hierarchical.cpp 70291075c00a9a557529c2562dedcfc6c6c3ec32
> src/master/allocator/mesos/metrics.hpp PRE-CREATION
> src/master/allocator/mesos/metrics.cpp PRE-CREATION
> src/tests/hierarchical_allocator_tests.cpp 3e4ad31925e1b815a74d67fa3962d23fa5bc89d1
>
> Diff: https://reviews.apache.org/r/43881/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 43881: Added allocator metric for the number of
allocations to a framework.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43881/
-----------------------------------------------------------
(Updated March 3, 2016, 5:17 p.m.)
Review request for mesos, Alexander Rukletsov and Ben Mahler.
Changes
-------
Addressed comments form Alex.
Bugs: MESOS-4719
https://issues.apache.org/jira/browse/MESOS-4719
Repository: mesos
Description (updated)
-------
Added allocator metric for the number of allocations to a framework.
Diffs (updated)
-----
docs/monitoring.md 827f7073204fcf8575ca980a5571c8be4f5e4110
src/master/allocator/mesos/hierarchical.cpp 70291075c00a9a557529c2562dedcfc6c6c3ec32
src/master/allocator/mesos/metrics.hpp PRE-CREATION
src/master/allocator/mesos/metrics.cpp PRE-CREATION
src/tests/hierarchical_allocator_tests.cpp 3e4ad31925e1b815a74d67fa3962d23fa5bc89d1
Diff: https://reviews.apache.org/r/43881/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 43881: Added allocator metric for the number of
allocations to a framework.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43881/
-----------------------------------------------------------
(Updated March 3, 2016, 2:11 p.m.)
Review request for mesos, Alexander Rukletsov and Ben Mahler.
Changes
-------
Tidied up includes.
Bugs: MESOS-4719
https://issues.apache.org/jira/browse/MESOS-4719
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
docs/monitoring.md 827f7073204fcf8575ca980a5571c8be4f5e4110
src/master/allocator/mesos/hierarchical.cpp 70291075c00a9a557529c2562dedcfc6c6c3ec32
src/master/allocator/mesos/metrics.hpp PRE-CREATION
src/master/allocator/mesos/metrics.cpp PRE-CREATION
src/tests/hierarchical_allocator_tests.cpp 3e4ad31925e1b815a74d67fa3962d23fa5bc89d1
Diff: https://reviews.apache.org/r/43881/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 43881: Added allocator metric for the number of
allocations to a framework.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43881/
-----------------------------------------------------------
(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-4719
https://issues.apache.org/jira/browse/MESOS-4719
Repository: mesos
Description (updated)
-------
See summary.
Diffs (updated)
-----
docs/monitoring.md 827f7073204fcf8575ca980a5571c8be4f5e4110
src/master/allocator/mesos/hierarchical.cpp 24fa50f62dec10ed43089297473cc386d6ba2f78
src/master/allocator/mesos/metrics.hpp PRE-CREATION
src/master/allocator/mesos/metrics.cpp PRE-CREATION
src/tests/hierarchical_allocator_tests.cpp 5f771f02db9bd098f3cd36730cd84bf2f5e87a33
Diff: https://reviews.apache.org/r/43881/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 43881: Added allocator metric for the number of
allocations to a framework.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43881/
-----------------------------------------------------------
(Updated March 2, 2016, 11:54 a.m.)
Review request for mesos, Alexander Rukletsov and Ben Mahler.
Changes
-------
Backtickified.
Bugs: MESOS-4719
https://issues.apache.org/jira/browse/MESOS-4719
Repository: mesos
Description
-------
Added allocator metric for the number of allocations to a framework.
Diffs (updated)
-----
docs/monitoring.md 827f7073204fcf8575ca980a5571c8be4f5e4110
src/master/allocator/mesos/hierarchical.cpp 24fa50f62dec10ed43089297473cc386d6ba2f78
src/master/allocator/mesos/metrics.hpp PRE-CREATION
src/tests/hierarchical_allocator_tests.cpp 5f771f02db9bd098f3cd36730cd84bf2f5e87a33
Diff: https://reviews.apache.org/r/43881/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 43881: Added allocator metric for the number of
allocations to a framework.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43881/
-----------------------------------------------------------
(Updated March 2, 2016, 11:34 a.m.)
Review request for mesos, Alexander Rukletsov and Ben Mahler.
Changes
-------
Added removal of metrics of removed frameworks; style fixes.
Bugs: MESOS-4719
https://issues.apache.org/jira/browse/MESOS-4719
Repository: mesos
Description
-------
Added allocator metric for the number of allocations to a framework.
Diffs (updated)
-----
docs/monitoring.md 827f7073204fcf8575ca980a5571c8be4f5e4110
src/master/allocator/mesos/hierarchical.cpp 24fa50f62dec10ed43089297473cc386d6ba2f78
src/master/allocator/mesos/metrics.hpp PRE-CREATION
src/tests/hierarchical_allocator_tests.cpp 5f771f02db9bd098f3cd36730cd84bf2f5e87a33
Diff: https://reviews.apache.org/r/43881/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 43881: Added allocator metric for the number of
allocations to a framework.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On March 1, 2016, 11:48 a.m., Alexander Rukletsov wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 268-272
> > <https://reviews.apache.org/r/43881/diff/9/?file=1273639#file1273639line268>
> >
> > Does it make sense to wrap framework-related metrics in a separate struct in metrics and hide `process::metrics::add` there? You can look here for inspiration: https://github.com/apache/mesos/blob/0fd95ccc54e4d144c3eb60e98bf77d53b6bdab63/src/master/metrics.hpp#L81
>
> Benjamin Bannier wrote:
> That's something I also thought about. This might be something useful more generally, so I suggest to look at this independently of this series and fix everywhere.
>
> What I find slightly suboptimal about the wrapper you linked to is that it would `add` and `remove` the same metric even when the wrapper is just copied (the `MetricProcess` uses just the name of the `Metric` to check identity). The calls `add` and `remove` return `Future<Nothing>`s to indicate success or failure but we cannot propagate that information in an RAII way in mesos (no exceptions). I think one would either need a type w/o copy ctr and a specific move ctr, or a more complicated infrastructure of e.g., handlers and shared metrics in the back.
Good point. I think we can leave it as is for now and follow up later. Have you synced with BenM about it?
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43881/#review121421
-----------------------------------------------------------
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/43881/
> -----------------------------------------------------------
>
> (Updated March 3, 2016, 4:17 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
>
>
> Bugs: MESOS-4719
> https://issues.apache.org/jira/browse/MESOS-4719
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added allocator metric for the number of allocations to a framework.
>
>
> Diffs
> -----
>
> docs/monitoring.md 827f7073204fcf8575ca980a5571c8be4f5e4110
> src/master/allocator/mesos/hierarchical.cpp 70291075c00a9a557529c2562dedcfc6c6c3ec32
> src/master/allocator/mesos/metrics.hpp PRE-CREATION
> src/master/allocator/mesos/metrics.cpp PRE-CREATION
> src/tests/hierarchical_allocator_tests.cpp 3e4ad31925e1b815a74d67fa3962d23fa5bc89d1
>
> Diff: https://reviews.apache.org/r/43881/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 43881: Added allocator metric for the number of
allocations to a framework.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On March 1, 2016, 12:48 p.m., Alexander Rukletsov wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 268-272
> > <https://reviews.apache.org/r/43881/diff/9/?file=1273639#file1273639line268>
> >
> > Does it make sense to wrap framework-related metrics in a separate struct in metrics and hide `process::metrics::add` there? You can look here for inspiration: https://github.com/apache/mesos/blob/0fd95ccc54e4d144c3eb60e98bf77d53b6bdab63/src/master/metrics.hpp#L81
That's something I also thought about. This might be something useful more generally, so I suggest to look at this independently of this series and fix everywhere.
What I find slightly suboptimal about the wrapper you linked to is that it would `add` and `remove` the same metric even when the wrapper is just copied (the `MetricProcess` uses just the name of the `Metric` to check identity). The calls `add` and `remove` return `Future<Nothing>`s to indicate success or failure but we cannot propagate that information in an RAII way in mesos (no exceptions). I think one would either need a type w/o copy ctr and a specific move ctr, or a more complicated infrastructure of e.g., handlers and shared metrics in the back.
> On March 1, 2016, 12:48 p.m., Alexander Rukletsov wrote:
> > src/tests/hierarchical_allocator_tests.cpp, line 2463
> > <https://reviews.apache.org/r/43881/diff/9/?file=1273640#file1273640line2463>
> >
> > I'm an ESL as well, but "to" sounds better than "towards" to me.
I choose *towards* since I am referring the *`framework1`'s quota*, but that might be an overly mechanic use of the language typical a German ESL user.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43881/#review121421
-----------------------------------------------------------
On March 2, 2016, 4:43 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43881/
> -----------------------------------------------------------
>
> (Updated March 2, 2016, 4:43 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
>
>
> Bugs: MESOS-4719
> https://issues.apache.org/jira/browse/MESOS-4719
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> docs/monitoring.md 827f7073204fcf8575ca980a5571c8be4f5e4110
> src/master/allocator/mesos/hierarchical.cpp 24fa50f62dec10ed43089297473cc386d6ba2f78
> src/master/allocator/mesos/metrics.hpp PRE-CREATION
> src/master/allocator/mesos/metrics.cpp PRE-CREATION
> src/tests/hierarchical_allocator_tests.cpp 5f771f02db9bd098f3cd36730cd84bf2f5e87a33
>
> Diff: https://reviews.apache.org/r/43881/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 43881: Added allocator metric for the number of
allocations to a framework.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43881/#review121421
-----------------------------------------------------------
src/master/allocator/mesos/hierarchical.hpp (lines 289 - 290)
<https://reviews.apache.org/r/43881/#comment183113>
Blank line, please.
src/master/allocator/mesos/hierarchical.cpp (lines 268 - 272)
<https://reviews.apache.org/r/43881/#comment183114>
Does it make sense to wrap framework-related metrics in a separate struct in metrics and hide `process::metrics::add` there? You can look here for inspiration: https://github.com/apache/mesos/blob/0fd95ccc54e4d144c3eb60e98bf77d53b6bdab63/src/master/metrics.hpp#L81
src/tests/hierarchical_allocator_tests.cpp (lines 2446 - 2447)
<https://reviews.apache.org/r/43881/#comment183115>
Blank line
src/tests/hierarchical_allocator_tests.cpp (line 2463)
<https://reviews.apache.org/r/43881/#comment183116>
I'm an ESL as well, but "to" sounds better than "towards" to me.
src/tests/hierarchical_allocator_tests.cpp (lines 2490 - 2493)
<https://reviews.apache.org/r/43881/#comment183118>
Let's not separate "clock" blocks. They signal about a certain relevant action.
- Alexander Rukletsov
On Feb. 29, 2016, 7:59 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43881/
> -----------------------------------------------------------
>
> (Updated Feb. 29, 2016, 7:59 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
>
>
> Bugs: MESOS-4719
> https://issues.apache.org/jira/browse/MESOS-4719
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added allocator metric for the number of allocations to a framework.
>
>
> 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/43881/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 43881: Added allocator metric for the number of
allocations to a framework.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On March 1, 2016, 12:29 a.m., Alexander Rojas wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 268-271
> > <https://reviews.apache.org/r/43881/diff/10/?file=1274385#file1274385line268>
> >
> > As in the previous review, I find much more readable:
> >
> > ```c++
> > metrics.framework_allocations.put(
> > frameworkId,
> > process::metrics::Counter(
> > strings::join(
> > "/",
> > "allocator/framework_allocations",
> > frameworkId.value())));
> > ```
Fixed with the same approach as in the previous review.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43881/#review121360
-----------------------------------------------------------
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/43881/
> -----------------------------------------------------------
>
> (Updated March 2, 2016, 11:34 a.m.)
>
>
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
>
>
> Bugs: MESOS-4719
> https://issues.apache.org/jira/browse/MESOS-4719
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added allocator metric for the number of allocations to a framework.
>
>
> Diffs
> -----
>
> docs/monitoring.md 827f7073204fcf8575ca980a5571c8be4f5e4110
> src/master/allocator/mesos/hierarchical.cpp 24fa50f62dec10ed43089297473cc386d6ba2f78
> src/master/allocator/mesos/metrics.hpp PRE-CREATION
> src/tests/hierarchical_allocator_tests.cpp 5f771f02db9bd098f3cd36730cd84bf2f5e87a33
>
> Diff: https://reviews.apache.org/r/43881/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 43881: Added allocator metric for the number of
allocations to a framework.
Posted by Alexander Rojas <al...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43881/#review121360
-----------------------------------------------------------
src/master/allocator/mesos/hierarchical.cpp (lines 268 - 271)
<https://reviews.apache.org/r/43881/#comment183032>
As in the previous review, I find much more readable:
```c++
metrics.framework_allocations.put(
frameworkId,
process::metrics::Counter(
strings::join(
"/",
"allocator/framework_allocations",
frameworkId.value())));
```
- 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/43881/
> -----------------------------------------------------------
>
> (Updated Feb. 29, 2016, 8:59 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
>
>
> Bugs: MESOS-4719
> https://issues.apache.org/jira/browse/MESOS-4719
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added allocator metric for the number of allocations to a framework.
>
>
> 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/43881/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 43881: Added allocator metric for the number of
allocations to a framework.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43881/
-----------------------------------------------------------
(Updated Feb. 29, 2016, 8:59 p.m.)
Review request for mesos, Alexander Rukletsov and Ben Mahler.
Changes
-------
Comment from Alexander.
Bugs: MESOS-4719
https://issues.apache.org/jira/browse/MESOS-4719
Repository: mesos
Description
-------
Added allocator metric for the number of allocations to a framework.
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/43881/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 43881: Added allocator metric for the number of
allocations to a framework.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43881/
-----------------------------------------------------------
(Updated Feb. 28, 2016, 10:28 p.m.)
Review request for mesos, Alexander Rukletsov and Ben Mahler.
Changes
-------
Addressed comments from Alexander.
Bugs: MESOS-4719
https://issues.apache.org/jira/browse/MESOS-4719
Repository: mesos
Description
-------
Added allocator metric for the number of allocations to a framework.
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/43881/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 43881: Added allocator metric for the number of
allocations to a framework.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On Feb. 27, 2016, 1:25 a.m., Alexander Rojas wrote:
> > src/tests/hierarchical_allocator_tests.cpp, lines 2432-2436
> > <https://reviews.apache.org/r/43881/diff/8/?file=1272155#file1272155line2432>
> >
> > This looks rather hard to read. I think a better solution would look like this:
> >
> > ```c++
> > std::string metricKey = strings::join("/", "allocator/framework_allocations", framework.id());
> >
> > EXPECT_EQ(1, metrics.values[metricKey]);
> > ```
Done.
> On Feb. 27, 2016, 1:25 a.m., Alexander Rojas wrote:
> > src/tests/hierarchical_allocator_tests.cpp, lines 2471-2480
> > <https://reviews.apache.org/r/43881/diff/8/?file=1272155#file1272155line2471>
> >
> > Same as above.
Done.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43881/#review121001
-----------------------------------------------------------
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/43881/
> -----------------------------------------------------------
>
> (Updated Feb. 28, 2016, 10:28 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
>
>
> Bugs: MESOS-4719
> https://issues.apache.org/jira/browse/MESOS-4719
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added allocator metric for the number of allocations to a framework.
>
>
> 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/43881/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 43881: Added allocator metric for the number of
allocations to a framework.
Posted by Alexander Rojas <al...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43881/#review121001
-----------------------------------------------------------
As with the previous one, I don't know whether this metric will help anyone. So can you add a use case in the JIRA?
src/tests/hierarchical_allocator_tests.cpp (lines 2432 - 2436)
<https://reviews.apache.org/r/43881/#comment182578>
This looks rather hard to read. I think a better solution would look like this:
```c++
std::string metricKey = strings::join("/", "allocator/framework_allocations", framework.id());
EXPECT_EQ(1, metrics.values[metricKey]);
```
src/tests/hierarchical_allocator_tests.cpp (lines 2471 - 2480)
<https://reviews.apache.org/r/43881/#comment182600>
Same as above.
- Alexander Rojas
On Feb. 26, 2016, 6:02 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43881/
> -----------------------------------------------------------
>
> (Updated Feb. 26, 2016, 6:02 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
>
>
> Bugs: MESOS-4719
> https://issues.apache.org/jira/browse/MESOS-4719
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added allocator metric for the number of allocations to a framework.
>
>
> 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/43881/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 43881: Added allocator metric for the number of
allocations to a framework.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43881/
-----------------------------------------------------------
(Updated Feb. 26, 2016, 6:02 p.m.)
Review request for mesos, Alexander Rukletsov and Ben Mahler.
Changes
-------
Rebased and get metrics without a helper.
Bugs: MESOS-4719
https://issues.apache.org/jira/browse/MESOS-4719
Repository: mesos
Description
-------
Added allocator metric for the number of allocations to a framework.
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/43881/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 43881: Added allocator metric for the number of
allocations to a framework.
Posted by Alexander Rojas <al...@mesosphere.io>.
> On Feb. 25, 2016, 5:30 p.m., Klaus Ma wrote:
> > src/master/allocator/mesos/hierarchical.hpp, line 376
> > <https://reviews.apache.org/r/43881/diff/7/?file=1270846#file1270846line376>
> >
> > We also need to remove counter in `removeFramework`; or we'll see metrics of removed framework.
>
> Benjamin Bannier wrote:
> Right now this is intended behavior, any preference @bmahler?
>
> Klaus Ma wrote:
> For the cluster running for a long time, there'll be several un-available metrics after doing operation.
> Just go through the other patches of metrics, similar comments on quota and filters.
I think I will join Klaus in this one. It makes no sense to keep gone frameworks appearing in the metrics for long times. But overall, I'm not even sure we need a metric at all.
Another solution would be to launch a timer when we remove the framework so it will show up for a time and then we remove it.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43881/#review120716
-----------------------------------------------------------
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/43881/
> -----------------------------------------------------------
>
> (Updated Feb. 28, 2016, 10:28 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
>
>
> Bugs: MESOS-4719
> https://issues.apache.org/jira/browse/MESOS-4719
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added allocator metric for the number of allocations to a framework.
>
>
> 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/43881/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 43881: Added allocator metric for the number of
allocations to a framework.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On Feb. 25, 2016, 5:30 p.m., Klaus Ma wrote:
> > src/master/allocator/mesos/hierarchical.hpp, line 376
> > <https://reviews.apache.org/r/43881/diff/7/?file=1270846#file1270846line376>
> >
> > We also need to remove counter in `removeFramework`; or we'll see metrics of removed framework.
Right now this is intended behavior, any preference @bmahler?
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43881/#review120716
-----------------------------------------------------------
On Feb. 26, 2016, 6:02 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43881/
> -----------------------------------------------------------
>
> (Updated Feb. 26, 2016, 6:02 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
>
>
> Bugs: MESOS-4719
> https://issues.apache.org/jira/browse/MESOS-4719
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added allocator metric for the number of allocations to a framework.
>
>
> 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/43881/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 43881: Added allocator metric for the number of
allocations to a framework.
Posted by Klaus Ma <kl...@gmail.com>.
> On Feb. 26, 2016, 12:30 a.m., Klaus Ma wrote:
> > src/master/allocator/mesos/hierarchical.hpp, line 376
> > <https://reviews.apache.org/r/43881/diff/7/?file=1270846#file1270846line376>
> >
> > We also need to remove counter in `removeFramework`; or we'll see metrics of removed framework.
>
> Benjamin Bannier wrote:
> Right now this is intended behavior, any preference @bmahler?
For the cluster running for a long time, there'll be several un-available metrics after doing operation.
Just go through the other patches of metrics, similar comments on quota and filters.
- Klaus
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43881/#review120716
-----------------------------------------------------------
On Feb. 27, 2016, 1:02 a.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43881/
> -----------------------------------------------------------
>
> (Updated Feb. 27, 2016, 1:02 a.m.)
>
>
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
>
>
> Bugs: MESOS-4719
> https://issues.apache.org/jira/browse/MESOS-4719
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added allocator metric for the number of allocations to a framework.
>
>
> 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/43881/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 43881: Added allocator metric for the number of
allocations to a framework.
Posted by Klaus Ma <kl...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43881/#review120716
-----------------------------------------------------------
src/master/allocator/mesos/hierarchical.hpp (line 376)
<https://reviews.apache.org/r/43881/#comment182180>
We also need to remove counter in `removeFramework`; or we'll see metrics of removed framework.
src/master/allocator/mesos/hierarchical.cpp (line 1496)
<https://reviews.apache.org/r/43881/#comment182179>
I'd suggest to do this in `addFramework` & `removeFramework`?
src/tests/hierarchical_allocator_tests.cpp (line 2447)
<https://reviews.apache.org/r/43881/#comment182181>
I think `1.0` is easier to read for other contributor.
src/tests/hierarchical_allocator_tests.cpp (line 2479)
<https://reviews.apache.org/r/43881/#comment182182>
ditto
- Klaus Ma
On Feb. 25, 2016, 7:37 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43881/
> -----------------------------------------------------------
>
> (Updated Feb. 25, 2016, 7:37 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
>
>
> Bugs: MESOS-4719
> https://issues.apache.org/jira/browse/MESOS-4719
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added allocator metric for the number of allocations to a framework.
>
>
> 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/43881/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 43881: Added allocator metric for the number of
allocations to a framework.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43881/
-----------------------------------------------------------
(Updated Feb. 25, 2016, 12:37 p.m.)
Review request for mesos, Alexander Rukletsov and Ben Mahler.
Changes
-------
Avoided anon namespaces and added a comment.
Bugs: MESOS-4719
https://issues.apache.org/jira/browse/MESOS-4719
Repository: mesos
Description
-------
Added allocator metric for the number of allocations to a framework.
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/43881/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 43881: Added allocator metric for the number of
allocations to a framework.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43881/
-----------------------------------------------------------
(Updated Feb. 25, 2016, 12:01 p.m.)
Review request for mesos, Alexander Rukletsov and Ben Mahler.
Changes
-------
Added documentation and ws.
Bugs: MESOS-4719
https://issues.apache.org/jira/browse/MESOS-4719
Repository: mesos
Description
-------
Added allocator metric for the number of allocations to a framework.
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/43881/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 43881: Added allocator metric for the number of
allocations to a framework.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On Feb. 25, 2016, 12:11 a.m., Alexander Rojas wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 1204
> > <https://reviews.apache.org/r/43881/diff/5/?file=1268420#file1268420line1204>
> >
> > Even though I really like the anonymous namespace, we don't use it for some reasons (I think the main one is that symbols are still exported, even though with mangled names).
> >
> > All in all, there is only one place in non automatically generated code which uses them (and I wonder how it managed).
> >
> > All this to say, I guess this function should be static.
> >
> > Moreover, does this need to be a free function? I much rather have a private method.
I introduced a type here so only an anon namespace would have prevented public linkage. I removed the anon namespace and documented the type now.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43881/#review120597
-----------------------------------------------------------
On Feb. 25, 2016, 12:37 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43881/
> -----------------------------------------------------------
>
> (Updated Feb. 25, 2016, 12:37 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
>
>
> Bugs: MESOS-4719
> https://issues.apache.org/jira/browse/MESOS-4719
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added allocator metric for the number of allocations to a framework.
>
>
> 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/43881/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 43881: Added allocator metric for the number of
allocations to a framework.
Posted by Alexander Rojas <al...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43881/#review120597
-----------------------------------------------------------
src/master/allocator/mesos/hierarchical.cpp (line 1204)
<https://reviews.apache.org/r/43881/#comment182061>
Even though I really like the anonymous namespace, we don't use it for some reasons (I think the main one is that symbols are still exported, even though with mangled names).
All in all, there is only one place in non automatically generated code which uses them (and I wonder how it managed).
All this to say, I guess this function should be static.
Moreover, does this need to be a free function? I much rather have a private method.
src/tests/hierarchical_allocator_tests.cpp (lines 2506 - 2514)
<https://reviews.apache.org/r/43881/#comment182063>
Could you add a small comment, akin to the one in line 2488 on what is happening here?
- Alexander Rojas
On Feb. 24, 2016, 9:37 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43881/
> -----------------------------------------------------------
>
> (Updated Feb. 24, 2016, 9:37 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
>
>
> Bugs: MESOS-4719
> https://issues.apache.org/jira/browse/MESOS-4719
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added allocator metric for the number of allocations to a framework.
>
>
> 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/43881/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 43881: Added allocator metric for the number of
allocations to a framework.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43881/
-----------------------------------------------------------
(Updated Feb. 24, 2016, 9:37 p.m.)
Review request for mesos, Alexander Rukletsov and Ben Mahler.
Changes
-------
Renamed variable.
Bugs: MESOS-4719
https://issues.apache.org/jira/browse/MESOS-4719
Repository: mesos
Description
-------
Added allocator metric for the number of allocations to a framework.
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/43881/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 43881: Added allocator metric for the number of
allocations to a framework.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43881/
-----------------------------------------------------------
(Updated Feb. 24, 2016, 11:49 a.m.)
Review request for mesos, Alexander Rukletsov and Ben Mahler.
Changes
-------
Just whitespace (no functional changes)
Bugs: MESOS-4719
https://issues.apache.org/jira/browse/MESOS-4719
Repository: mesos
Description
-------
Added allocator metric for the number of allocations to a framework.
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/43881/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 43881: Added allocator metric for the number of
allocations to a framework.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43881/
-----------------------------------------------------------
(Updated Feb. 24, 2016, 11:22 a.m.)
Review request for mesos, Alexander Rukletsov and Ben Mahler.
Bugs: MESOS-4719
https://issues.apache.org/jira/browse/MESOS-4719
Repository: mesos
Description
-------
Added allocator metric for the number of allocations to a framework.
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/43881/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 43881: Added allocator metric for the number of
allocations to a framework.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43881/
-----------------------------------------------------------
(Updated Feb. 24, 2016, 10:18 a.m.)
Review request for mesos, Alexander Rukletsov and Ben Mahler.
Bugs: MESOS-4719
https://issues.apache.org/jira/browse/MESOS-4719
Repository: mesos
Description
-------
Added allocator metric for the number of allocations to a framework.
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/43881/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