You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Meng Zhu <mz...@mesosphere.io> on 2018/09/04 23:25:00 UTC
Re: Review Request 68548: Added allocator benchmark test harness.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68548/#review208321
-----------------------------------------------------------
For the description, let's be more informative, how about:
Introduced a base class for writing allocator benchmarks.
The test harness encapsulates the common logic in the allocator
benchmarks such has adding frameworks and agents. One only
needs to customize the various options in the `Benchmarkconfig`
to set up the test cluster.
src/tests/hierarchical_allocator_benchmarks.cpp
Lines 141 (patched)
<https://reviews.apache.org/r/68548/#comment292155>
Since we are touching the allocator option and random sorter is probably interesting for the benchmark you added in https://reviews.apache.org/r/68591, can you customize this using the helper here:
https://github.com/apache/mesos/blob/master/src/master/allocator/allocator.cpp#L35
I think you might need to introduce a new option in the `Bencmarkconfig`.
src/tests/hierarchical_allocator_benchmarks.cpp
Lines 143 (patched)
<https://reviews.apache.org/r/68548/#comment292159>
Let's add a comment here regarding what this is for, and show an example of framework/agent id.
src/tests/hierarchical_allocator_benchmarks.cpp
Lines 182 (patched)
<https://reviews.apache.org/r/68548/#comment292172>
Ah, I did not expect this, my bad.
Initialize will unpause the allocator:
https://github.com/apache/mesos/blob/6705b31187d2441cb62670bd0be4904896c8b695/src/master/allocator/mesos/hierarchical.cpp#L170
So we need to pause after initializing.
Per my comments above regarding `create()` the allocator in the base class, it no longer makes sense to ask the user to pause. So let's:
(1) create the allocator here according to the config
(2) initialize it
(3) pause it
(4) add frameworks and agents.
src/tests/hierarchical_allocator_benchmarks.cpp
Lines 184 (patched)
<https://reviews.apache.org/r/68548/#comment292154>
`CHECK_NOTNONE` instead of unguarded get.
src/tests/hierarchical_allocator_benchmarks.cpp
Lines 202 (patched)
<https://reviews.apache.org/r/68548/#comment292160>
hmm, I thought we removed the `replace` call along with the restriction that the user needs to name their profiles `...-#`. I bet this will lead to some grumpy debugging sessions:)
Can we just let users name their profiles at will and just concatenate the delineator?
e.g. I can just name my framework profile "framework" and you are free to process it to "framework-3".
I think most users would not care about the naming, as long as the naming is not surprising and documented, it should be fine.
ditto for the agent.
src/tests/hierarchical_allocator_benchmarks.cpp
Lines 255-256 (patched)
<https://reviews.apache.org/r/68548/#comment292156>
One thing that is missing for the existing benchmarks is agent's used resources, e.g.:
https://github.com/apache/mesos/blob/6f5ff9550d975ed0adc2a12f7690a52be3a47afa/src/tests/hierarchical_allocator_tests.cpp#L6253-L6268
Can you add that option as well? Let's try to make the prototype usable for the existing benchmarks.
src/tests/hierarchical_allocator_benchmarks.cpp
Lines 278-279 (patched)
<https://reviews.apache.org/r/68548/#comment292164>
I feel there are some ambiguities as to which parameters are kept/copied in the benchmark base class, which are kept just in the `Benchmarkconfig`, e.g. it is not clear to me why we keep the "profiles" here. They are not needed after the initialization phase.
I wonder should we just keep a copy of the benchmarkconfig here?
Any better idea?
src/tests/hierarchical_allocator_benchmarks.cpp
Lines 281-282 (patched)
<https://reviews.apache.org/r/68548/#comment292158>
Add `const` to both.
- Meng Zhu
On Aug. 28, 2018, 12:45 p.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68548/
> -----------------------------------------------------------
>
> (Updated Aug. 28, 2018, 12:45 p.m.)
>
>
> Review request for mesos, Meng Zhu and Till Toenshoff.
>
>
> Bugs: MESOS-9187
> https://issues.apache.org/jira/browse/MESOS-9187
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The new harness allows us to run allocators with different framework and
> agent profiles.
>
>
> Diffs
> -----
>
> src/Makefile.am cf0cf22b3f582d3d4427f13288af3845aef45263
> src/tests/hierarchical_allocator_benchmarks.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/68548/diff/4/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Kapil Arya
>
>
Re: Review Request 68548: Introduced a base class for writing
allocator benchmarks.
Posted by Kapil Arya <ka...@mesosphere.io>.
> On Sept. 4, 2018, 7:25 p.m., Meng Zhu wrote:
> > src/tests/hierarchical_allocator_benchmarks.cpp
> > Lines 143 (patched)
> > <https://reviews.apache.org/r/68548/diff/4/?file=2079338#file2079338line143>
> >
> > Let's add a comment here regarding what this is for, and show an example of framework/agent id.
Just removed this entirely for frameworks/agents. Added a separate flag for unique roles.
- Kapil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68548/#review208321
-----------------------------------------------------------
On Sept. 11, 2018, 3:15 p.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68548/
> -----------------------------------------------------------
>
> (Updated Sept. 11, 2018, 3:15 p.m.)
>
>
> Review request for mesos, Meng Zhu and Till Toenshoff.
>
>
> Bugs: MESOS-9187
> https://issues.apache.org/jira/browse/MESOS-9187
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The test harness encapsulates the common logic in the allocator
> benchmarks such has adding frameworks and agents. One only
> needs to customize the various options in the `Benchmarkconfig`
> to set up the test cluster.
>
>
> Diffs
> -----
>
> src/Makefile.am 0a9eeabe52b447a311de22f9772c7e90b31a6064
> src/tests/hierarchical_allocator_benchmarks.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/68548/diff/5/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Kapil Arya
>
>
Re: Review Request 68548: Introduced a base class for writing
allocator benchmarks.
Posted by Kapil Arya <ka...@mesosphere.io>.
> On Sept. 4, 2018, 7:25 p.m., Meng Zhu wrote:
> > src/tests/hierarchical_allocator_benchmarks.cpp
> > Lines 278-279 (patched)
> > <https://reviews.apache.org/r/68548/diff/4/?file=2079338#file2079338line278>
> >
> > I feel there are some ambiguities as to which parameters are kept/copied in the benchmark base class, which are kept just in the `Benchmarkconfig`, e.g. it is not clear to me why we keep the "profiles" here. They are not needed after the initialization phase.
> >
> > I wonder should we just keep a copy of the benchmarkconfig here?
> >
> > Any better idea?
For now, I moved it into the BenchmarkConfig. I'm not sure if there's a better place for it, but sounds reasonable to me for now.
- Kapil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68548/#review208321
-----------------------------------------------------------
On Sept. 11, 2018, 3:15 p.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68548/
> -----------------------------------------------------------
>
> (Updated Sept. 11, 2018, 3:15 p.m.)
>
>
> Review request for mesos, Meng Zhu and Till Toenshoff.
>
>
> Bugs: MESOS-9187
> https://issues.apache.org/jira/browse/MESOS-9187
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The test harness encapsulates the common logic in the allocator
> benchmarks such has adding frameworks and agents. One only
> needs to customize the various options in the `Benchmarkconfig`
> to set up the test cluster.
>
>
> Diffs
> -----
>
> src/Makefile.am 0a9eeabe52b447a311de22f9772c7e90b31a6064
> src/tests/hierarchical_allocator_benchmarks.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/68548/diff/5/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Kapil Arya
>
>