You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Benjamin Mahler <bm...@apache.org> on 2019/01/03 18:06:12 UTC
Re: Review Request 69603: Extended `min_allocatable_resources` flag
to cover non-scalar resources.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69603/#review211642
-----------------------------------------------------------
docs/configuration/master.md
Line 210 (original), 210 (patched)
<https://reviews.apache.org/r/69603/#comment297038>
sets of resource quantities
docs/configuration/master.md
Lines 221 (patched)
<https://reviews.apache.org/r/69603/#comment297039>
at least
include/mesos/allocator/allocator.hpp
Line 53 (original), 55 (patched)
<https://reviews.apache.org/r/69603/#comment297040>
Ditto comment in earlier review, no need for the quantities namespace
src/master/allocator/mesos/hierarchical.cpp
Lines 2434-2440 (original), 2434-2440 (patched)
<https://reviews.apache.org/r/69603/#comment297043>
Is there a way (e.g. benchmark) to check that we didn't make this contains slower?
src/master/master.cpp
Lines 747-748 (original), 739-740 (patched)
<https://reviews.apache.org/r/69603/#comment297042>
Based on my other comment, more explanation is needed here about what this is for and whether it's accomplishing it.
src/master/master.cpp
Lines 753-754 (original), 745-746 (patched)
<https://reviews.apache.org/r/69603/#comment297041>
We call these "first class" rather than "common", but why do you want this logic?
Maybe you intended to warn about potentially "scarce" (i.e. "non-ubiquitous") resources being used in the flag? Like "gpus"? Or "someCustomResourceOnlyOnSomeMachines"?
src/tests/master_allocator_tests.cpp
Lines 1287-1290 (original), 1288-1291 (patched)
<https://reviews.apache.org/r/69603/#comment297045>
Maybe you can line up these contains comments to make it a bit more readable?
- Benjamin Mahler
On Dec. 20, 2018, 7 a.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69603/
> -----------------------------------------------------------
>
> (Updated Dec. 20, 2018, 7 a.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Bugs: MESOS-9324
> https://issues.apache.org/jira/browse/MESOS-9324
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The master flag `min_allocatable_resources` currently only supports
> scalar resources. Sometimes it is desirable to control the quantities
> of non-scalar resources in the offer as well, for example, to
> ensure offers always contain port resources.
>
> This patch extended the flag to cover non-scalar resources.
> For `RANGES` and `SET` type resources, their quantities are the
> number of different instances in the range or set. For example,
> `range:[1-5]` has a quantity of 5 and `set:{a,b}` has a quantity of 2.
>
> This patch also updated affected tests and modified the existing
> test for `min_allocatable_resources` to cover the updated use cases.
>
>
> Diffs
> -----
>
> docs/configuration/master.md 83b83b0b7a41f982c8680cee21f0971a0877e49e
> include/mesos/allocator/allocator.hpp 2a6849bf698f171fc31c84165bd574b8f4f846ea
> src/master/allocator/mesos/hierarchical.cpp cc8ab919aedb33cf424edfdb622ca13a2cc8ff0f
> src/master/flags.cpp f9b68bcd5668160a7e90d6569f69f2713df802fe
> src/master/master.cpp b4faf2b077a0288ba36195b7a21402932489d316
> src/tests/hierarchical_allocator_benchmarks.cpp 527a25d1414882012f3906370bd1a2a7b40a92f2
> src/tests/hierarchical_allocator_tests.cpp a3a6d7bb956a6d16f01e2c312d69f704fcbe9ddd
> src/tests/master_allocator_tests.cpp 9dbab1822ea83ac912a56eb3196060c648a2a672
>
>
> Diff: https://reviews.apache.org/r/69603/diff/1/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Meng Zhu
>
>
Re: Review Request 69603: Extended `min_allocatable_resources` flag
to cover non-scalar resources.
Posted by Meng Zhu <mz...@mesosphere.io>.
> On Jan. 3, 2019, 10:06 a.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp
> > Lines 2434-2440 (original), 2434-2440 (patched)
> > <https://reviews.apache.org/r/69603/diff/1/?file=2115201#file2115201line2435>
> >
> > Is there a way (e.g. benchmark) to check that we didn't make this contains slower?
Unlikely (previously, it involved copying the whole resource object for subtraction), we now only copy a double. But I can run the allocator performance benchmark later to confirm.
> On Jan. 3, 2019, 10:06 a.m., Benjamin Mahler wrote:
> > src/master/master.cpp
> > Lines 753-754 (original), 745-746 (patched)
> > <https://reviews.apache.org/r/69603/diff/1/?file=2115203#file2115203line755>
> >
> > We call these "first class" rather than "common", but why do you want this logic?
> >
> > Maybe you intended to warn about potentially "scarce" (i.e. "non-ubiquitous") resources being used in the flag? Like "gpus"? Or "someCustomResourceOnlyOnSomeMachines"?
I was mainly thinking of typo, imagine the flag is set to cpu:... (instead of "cpus"), then no offer will be made and it could take quite some time to debug.
> On Jan. 3, 2019, 10:06 a.m., Benjamin Mahler wrote:
> > src/tests/master_allocator_tests.cpp
> > Lines 1287-1290 (original), 1288-1291 (patched)
> > <https://reviews.apache.org/r/69603/diff/1/?file=2115206#file2115206line1288>
> >
> > Maybe you can line up these contains comments to make it a bit more readable?
Done.
- Meng
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69603/#review211642
-----------------------------------------------------------
On Jan. 4, 2019, 12:54 p.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69603/
> -----------------------------------------------------------
>
> (Updated Jan. 4, 2019, 12:54 p.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Bugs: MESOS-9324
> https://issues.apache.org/jira/browse/MESOS-9324
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The master flag min_allocatable_resources currently only supports
> scalar resources. Sometimes it is desirable to control the quantities
> of non-scalar resources in the offer as well, for example, to
> ensure offers always contain port resources.
>
> This patch extended the flag to cover non-scalar resources.
> For RANGES and SET type resources, their quantities are the
> number of different instances in the range or set. For example,
> range:[1-5] has a quantity of 5 and set:{a,b} has a quantity of 2.
>
> This patch also updated affected tests and modified the existing
> test for min_allocatable_resources to cover the updated use cases.
>
>
> Diffs
> -----
>
> docs/configuration/master.md 83b83b0b7a41f982c8680cee21f0971a0877e49e
> include/mesos/allocator/allocator.hpp 2a6849bf698f171fc31c84165bd574b8f4f846ea
> src/master/allocator/mesos/hierarchical.cpp cc8ab919aedb33cf424edfdb622ca13a2cc8ff0f
> src/master/flags.cpp f9b68bcd5668160a7e90d6569f69f2713df802fe
> src/master/master.cpp b4faf2b077a0288ba36195b7a21402932489d316
> src/tests/hierarchical_allocator_benchmarks.cpp 527a25d1414882012f3906370bd1a2a7b40a92f2
> src/tests/hierarchical_allocator_tests.cpp a3a6d7bb956a6d16f01e2c312d69f704fcbe9ddd
> src/tests/master_allocator_tests.cpp 9dbab1822ea83ac912a56eb3196060c648a2a672
>
>
> Diff: https://reviews.apache.org/r/69603/diff/2/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Meng Zhu
>
>