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

[Benjamin Bannier]

> On Feb. 24, 2016, 2:26 p.m., Guangya Liu wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 1077-1080
> > 
> >
> > I think that we cannot say that the quota here is satisfied, but those 
> > are just quotas that being used, some quotas may still starve.
> > 
> > The following logic here 
> > https://github.com/apache/mesos/blob/master/src/master/allocator/mesos/hierarchical.cpp#L1230
> >  is checking if a quota is satisified or not.
> > 
> > Also for the comments in line 1079, what about use following which 
> > seems more accurate because the quota also support allocating reserved 
> > resource for now. (Though non revocable resources also include reserved 
> > resources, but I think that identifying the reserved resources here may be 
> > more clear.)
> > 
> > `Quota is satisfied with non-revocable and reserved resources.`

OK, I can see how someone might want to interpret *satisfied* as a binary, 
switched to *used* instead.

Re:l.1079, the comment reflects the current assumption that a quota is always 
satisfied with non-revocable resources. That assumption is then used below when 
the metric value is calculated. The comment is intended to just explain the 
assumptions made *here*.


- Benjamin


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


On Feb. 24, 2016, 11:47 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43884/
> ---
> 
> (Updated Feb. 24, 2016, 11:47 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
> 
> 
> Bugs: MESOS-4723
> https://issues.apache.org/jira/browse/MESOS-4723
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added allocator metrics for satisfied quotas.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> 3043888630b066505410d3b32c5b3f813cc458c1 
>   src/master/allocator/mesos/hierarchical.cpp 
> 5ef29f26ec8071f79c2f4f78dbe2bb0a613cc92d 
>   src/tests/hierarchical_allocator_tests.cpp 
> 5f771f02db9bd098f3cd36730cd84bf2f5e87a33 
> 
> Diff: https://reviews.apache.org/r/43884/diff/
> 
> 
> Testing
> ---
> 
> make check (OS X)
> 
> I confirmed that this does not lead to general performance regressions in the 
> allocator; this is partially expected since the added code only inserts 
> metrics in the allocator while the actual work is perform asynchronously. 
> These tests where performed with 
> `HierarchicalAllocator_BENCHMARK_Test.DeclineOffers` on an optimized build 
> under OS X using clang(trunk) as compiler.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

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

[Guangya Liu]

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




src/master/allocator/mesos/hierarchical.cpp (lines 1077 - 1080)


I think that we cannot say that the quota here is satisfied, but those are 
just quotas that being used, some quotas may still starve.

The following logic here 
https://github.com/apache/mesos/blob/master/src/master/allocator/mesos/hierarchical.cpp#L1230
 is checking if a quota is satisified or not.

Also for the comments in line 1079, what about use following which seems 
more accurate because the quota also support allocating reserved resource for 
now. (Though non revocable resources also include reserved resources, but I 
think that identifying the reserved resources here may be more clear.)

`Quota is satisfied with non-revocable and reserved resources.`


- Guangya Liu


On 二月 24, 2016, 10:47 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43884/
> ---
> 
> (Updated 二月 24, 2016, 10:47 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
> 
> 
> Bugs: MESOS-4723
> https://issues.apache.org/jira/browse/MESOS-4723
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added allocator metrics for satisfied quotas.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> 3043888630b066505410d3b32c5b3f813cc458c1 
>   src/master/allocator/mesos/hierarchical.cpp 
> 5ef29f26ec8071f79c2f4f78dbe2bb0a613cc92d 
>   src/tests/hierarchical_allocator_tests.cpp 
> 5f771f02db9bd098f3cd36730cd84bf2f5e87a33 
> 
> Diff: https://reviews.apache.org/r/43884/diff/
> 
> 
> Testing
> ---
> 
> make check (OS X)
> 
> I confirmed that this does not lead to general performance regressions in the 
> allocator; this is partially expected since the added code only inserts 
> metrics in the allocator while the actual work is perform asynchronously. 
> These tests where performed with 
> `HierarchicalAllocator_BENCHMARK_Test.DeclineOffers` on an optimized build 
> under OS X using clang(trunk) as compiler.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

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

[Mesos ReviewBot]

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



Patch looks great!

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

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

- Mesos ReviewBot


On Feb. 24, 2016, 10:47 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43884/
> ---
> 
> (Updated Feb. 24, 2016, 10:47 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
> 
> 
> Bugs: MESOS-4723
> https://issues.apache.org/jira/browse/MESOS-4723
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added allocator metrics for satisfied quotas.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> 3043888630b066505410d3b32c5b3f813cc458c1 
>   src/master/allocator/mesos/hierarchical.cpp 
> 5ef29f26ec8071f79c2f4f78dbe2bb0a613cc92d 
>   src/tests/hierarchical_allocator_tests.cpp 
> 5f771f02db9bd098f3cd36730cd84bf2f5e87a33 
> 
> Diff: https://reviews.apache.org/r/43884/diff/
> 
> 
> Testing
> ---
> 
> make check (OS X)
> 
> I confirmed that this does not lead to general performance regressions in the 
> allocator; this is partially expected since the added code only inserts 
> metrics in the allocator while the actual work is perform asynchronously. 
> These tests where performed with 
> `HierarchicalAllocator_BENCHMARK_Test.DeclineOffers` on an optimized build 
> under OS X using clang(trunk) as compiler.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

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

[Benjamin Bannier]

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

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


Review request for mesos, Alexander Rukletsov and Ben Mahler.


Changes
---

Just whitespace (no functional changes)


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


Repository: mesos


Description
---

Added allocator metrics for satisfied quotas.


Diffs (updated)
-

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

Diff: https://reviews.apache.org/r/43884/diff/


Testing
---

make check (OS X)

I confirmed that this does not lead to general performance regressions in the 
allocator; this is partially expected since the added code only inserts metrics 
in the allocator while the actual work is perform asynchronously. These tests 
where performed with `HierarchicalAllocator_BENCHMARK_Test.DeclineOffers` on an 
optimized build under OS X using clang(trunk) as compiler.


Thanks,

Benjamin Bannier

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

[Benjamin Bannier]

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

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


Review request for mesos, Alexander Rukletsov and Ben Mahler.


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


Repository: mesos


Description
---

Added allocator metrics for satisfied quotas.


Diffs (updated)
-

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

Diff: https://reviews.apache.org/r/43884/diff/


Testing
---

make check (OS X)

I confirmed that this does not lead to general performance regressions in the 
allocator; this is partially expected since the added code only inserts metrics 
in the allocator while the actual work is perform asynchronously. These tests 
where performed with `HierarchicalAllocator_BENCHMARK_Test.DeclineOffers` on an 
optimized build under OS X using clang(trunk) as compiler.


Thanks,

Benjamin Bannier

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

[Benjamin Bannier]

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

(Updated Feb. 24, 2016, 10:20 a.m.)


Review request for mesos, Alexander Rukletsov and Ben Mahler.


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


Repository: mesos


Description
---

Added allocator metrics for satisfied quotas.


Diffs (updated)
-

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

Diff: https://reviews.apache.org/r/43884/diff/


Testing
---

make check (OS X)

I confirmed that this does not lead to general performance regressions in the 
allocator; this is partially expected since the added code only inserts metrics 
in the allocator while the actual work is perform asynchronously. These tests 
where performed with `HierarchicalAllocator_BENCHMARK_Test.DeclineOffers` on an 
optimized build under OS X using clang(trunk) as compiler.


Thanks,

Benjamin Bannier

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

[Mesos ReviewBot]

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



Patch looks great!

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

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

- Mesos ReviewBot


On Feb. 23, 2016, 5:08 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43884/
> ---
> 
> (Updated Feb. 23, 2016, 5:08 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
> 
> 
> Bugs: MESOS-4723
> https://issues.apache.org/jira/browse/MESOS-4723
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added allocator metrics for satisfied quotas.
> 
> 
> 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/43884/diff/
> 
> 
> Testing
> ---
> 
> make check (OS X)
> 
> I confirmed that this does not lead to general performance regressions in the 
> allocator; this is partially expected since the added code only inserts 
> metrics in the allocator while the actual work is perform asynchronously. 
> These tests where performed with 
> `HierarchicalAllocator_BENCHMARK_Test.DeclineOffers` on an optimized build 
> under OS X using clang(trunk) as compiler.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Review Request 43884: Added allocator metrics for satisfied quotas.

[Benjamin Bannier]

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

Review request for mesos, Alexander Rukletsov and Ben Mahler.


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


Repository: mesos


Description
---

Added allocator metrics for satisfied quotas.


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


Testing
---

make check (OS X)

I confirmed that this does not lead to general performance regressions in the 
allocator; this is partially expected since the added code only inserts metrics 
in the allocator while the actual work is perform asynchronously. These tests 
where performed with `HierarchicalAllocator_BENCHMARK_Test.DeclineOffers` on an 
optimized build under OS X using clang(trunk) as compiler.


Thanks,

Benjamin Bannier