Re: Review Request 43881: Added allocator metric for the number of allocations to a framework.

2016-03-21 Thread Benjamin Bannier 

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

2016-03-15 Thread Benjamin Bannier 

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

2016-03-04 Thread Benjamin Bannier 


> On March 4, 2016, 2:05 p.m., Alexander Rukletsov wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 275-280
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > Backtick `framework1` please.

Done.


> On March 4, 2016, 2:05 p.m., Alexander Rukletsov wrote:
> > src/tests/hierarchical_allocator_tests.cpp, lines 2464-2472
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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.

2016-03-04 Thread Alexander Rukletsov 

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


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)


Maybe here a helper as well.



src/master/allocator/mesos/hierarchical.cpp (lines 1498 - 1499)


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)


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)


Backtick `framework1` please.



src/tests/hierarchical_allocator_tests.cpp (lines 2464 - 2472)


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)


Feel free to kill this line



src/tests/hierarchical_allocator_tests.cpp (lines 2492 - 2495)


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.

2016-03-04 Thread Alexander Rukletsov 


> On March 1, 2016, 11:48 a.m., Alexander Rukletsov wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 268-272
> > 
> >
> > 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`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.

2016-03-03 Thread Benjamin Bannier 

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

2016-03-03 Thread Benjamin Bannier 

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

2016-03-02 Thread Benjamin Bannier 


> On March 1, 2016, 12:48 p.m., Alexander Rukletsov wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 268-272
> > 
> >
> > 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`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
> > 
> >
> > 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.

2016-03-02 Thread Benjamin Bannier 

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

2016-03-02 Thread Benjamin Bannier 

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

2016-03-02 Thread Benjamin Bannier 


> On March 1, 2016, 12:29 a.m., Alexander Rojas wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 268-271
> > 
> >
> > 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.

2016-03-02 Thread Benjamin Bannier 

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

2016-03-01 Thread Guangya Liu 

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


no difference



src/tests/hierarchical_allocator_tests.cpp (line 2463)


s/agent2/`agent2`
s/framework1/`framework1`



src/tests/hierarchical_allocator_tests.cpp (line 2464)


s/. Framework2 /, `framework2`



src/tests/hierarchical_allocator_tests.cpp (line 2496)


s/agent2/`agent2`
s/framework2/`framework2`



src/tests/hierarchical_allocator_tests.cpp (line 2499)


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.

2016-03-01 Thread Alexander Rukletsov 


> On Feb. 29, 2016, 5:08 p.m., Alexander Rojas wrote:
> > src/tests/hierarchical_allocator_tests.cpp, lines 2498-2502
> > 
> >
> > 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.

2016-02-29 Thread Alexander Rojas 

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


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.

2016-02-29 Thread Alexander Rojas 

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


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.

2016-02-29 Thread Benjamin Bannier 


> On Feb. 29, 2016, 6:08 p.m., Alexander Rojas wrote:
> > src/tests/hierarchical_allocator_tests.cpp, lines 2498-2502
> > 
> >
> > 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.

2016-02-29 Thread Benjamin Bannier 

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

2016-02-29 Thread Alexander Rojas 

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


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.

2016-02-29 Thread Alexander Rojas 


> On Feb. 25, 2016, 5:30 p.m., Klaus Ma wrote:
> > src/master/allocator/mesos/hierarchical.hpp, line 376
> > 
> >
> > 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.

2016-02-28 Thread Benjamin Bannier 


> On Feb. 27, 2016, 1:25 a.m., Alexander Rojas wrote:
> > src/tests/hierarchical_allocator_tests.cpp, lines 2432-2436
> > 
> >
> > 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
> > 
> >
> > 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.

2016-02-28 Thread Benjamin Bannier 

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

2016-02-28 Thread Klaus Ma 


> On Feb. 26, 2016, 12:30 a.m., Klaus Ma wrote:
> > src/master/allocator/mesos/hierarchical.hpp, line 376
> > 
> >
> > 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.

2016-02-26 Thread Alexander Rojas 

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


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)


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.

2016-02-26 Thread Benjamin Bannier 


> On Feb. 25, 2016, 5:30 p.m., Klaus Ma wrote:
> > src/master/allocator/mesos/hierarchical.hpp, line 376
> > 
> >
> > 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.

2016-02-26 Thread Benjamin Bannier 

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

2016-02-25 Thread Klaus Ma 

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


We also need to remove counter in `removeFramework`; or we'll see metrics 
of removed framework.



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


I'd suggest to do this in `addFramework` & `removeFramework`?



src/tests/hierarchical_allocator_tests.cpp (line 2447)


I think `1.0` is easier to read for other contributor.



src/tests/hierarchical_allocator_tests.cpp (line 2479)


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.

2016-02-25 Thread Benjamin Bannier 


> On Feb. 25, 2016, 12:11 a.m., Alexander Rojas wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 1204
> > 
> >
> > 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.

2016-02-25 Thread Benjamin Bannier 

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

2016-02-25 Thread Benjamin Bannier 

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

2016-02-24 Thread Alexander Rojas 

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


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)


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.

2016-02-24 Thread Benjamin Bannier 

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

2016-02-24 Thread Guangya Liu 


> On 二月 24, 2016, 12:53 a.m., Guangya Liu wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 1354
> > 
> >
> > 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.

2016-02-24 Thread Benjamin Bannier 

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

2016-02-24 Thread Benjamin Bannier 


> On Feb. 24, 2016, 1:53 a.m., Guangya Liu wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 1354
> > 
> >
> > 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.

2016-02-24 Thread Benjamin Bannier 

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

2016-02-24 Thread Guangya Liu 


> On 二月 24, 2016, 12:53 a.m., Guangya Liu wrote:
> > src/tests/hierarchical_allocator_tests.cpp, lines 2369-2370
> > 
> >
> > 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.

2016-02-24 Thread Benjamin Bannier 

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

Re: Review Request 43881: Added allocator metric for the number of allocations to a framework.

2016-02-24 Thread Benjamin Bannier 


> On Feb. 24, 2016, 1:53 a.m., Guangya Liu wrote:
> > src/tests/hierarchical_allocator_tests.cpp, lines 2369-2370
> > 
> >
> > 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.

2016-02-23 Thread Guangya Liu 

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


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)


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)


4 spaces



src/tests/hierarchical_allocator_tests.cpp (lines 2369 - 2370)


What about following format?

allocator->setQuota(
framework1.role(),
createQuota(framework1.role(),
"disk:10"));



src/tests/hierarchical_allocator_tests.cpp (line 2371)


s/Setting/Setting a



src/tests/hierarchical_allocator_tests.cpp (lines 2390 - 2395)


4 spaces



src/tests/hierarchical_allocator_tests.cpp (lines 2400 - 2403)


4 spaces



src/tests/hierarchical_allocator_tests.cpp (lines 2416 - 2418)


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

Review Request 43881: Added allocator metric for the number of allocations to a framework.

2016-02-23 Thread Benjamin Bannier 

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