You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Kapil Arya <ka...@mesosphere.io> on 2018/08/28 19:45:41 UTC
Review Request 68548: Added allocator benchmark test harness.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68548/
-----------------------------------------------------------
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/1/
Testing
-------
Thanks,
Kapil Arya
Re: Review Request 68548: Introduced a base class for writing
allocator benchmarks.
Posted by Meng Zhu <mz...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68548/#review208611
-----------------------------------------------------------
As discussed offline, let's introduce two constructs: one for the static/read-only framework profile; another to keep track of individual framework run-time/dynamic states.
src/tests/hierarchical_allocator_benchmarks.cpp
Lines 237 (patched)
<https://reviews.apache.org/r/68548/#comment292705>
Let's add a comment here about why we are pausing the allocator:
We pause the allocator here to prevent any event-driven allocations while adding frameworks.
- Meng Zhu
On Sept. 13, 2018, 1:22 p.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68548/
> -----------------------------------------------------------
>
> (Updated Sept. 13, 2018, 1:22 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/6/
>
>
> 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. 14, 2018, 3:07 p.m., Meng Zhu wrote:
> > src/tests/hierarchical_allocator_benchmarks.cpp
> > Lines 306 (patched)
> > <https://reviews.apache.org/r/68548/diff/8/?file=2089055#file2089055line306>
> >
> > const ?
The value is computed during initialize() call.
- Kapil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68548/#review208633
-----------------------------------------------------------
On Sept. 14, 2018, 3:16 p.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68548/
> -----------------------------------------------------------
>
> (Updated Sept. 14, 2018, 3:16 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/9/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Kapil Arya
>
>
Re: Review Request 68548: Introduced a base class for writing
allocator benchmarks.
Posted by Meng Zhu <mz...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68548/#review208633
-----------------------------------------------------------
Fix it, then Ship it!
This is great!
src/tests/hierarchical_allocator_benchmarks.cpp
Lines 297 (patched)
<https://reviews.apache.org/r/68548/#comment292743>
reference: const FrameworkProfile&
src/tests/hierarchical_allocator_benchmarks.cpp
Lines 306 (patched)
<https://reviews.apache.org/r/68548/#comment292746>
const ?
- Meng Zhu
On Sept. 14, 2018, 11 a.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68548/
> -----------------------------------------------------------
>
> (Updated Sept. 14, 2018, 11 a.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/8/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Kapil Arya
>
>
Re: Review Request 68548: Introduced a base class for writing
allocator benchmarks.
Posted by Kapil Arya <ka...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68548/
-----------------------------------------------------------
(Updated Sept. 14, 2018, 3:16 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 (updated)
-----
src/Makefile.am 0a9eeabe52b447a311de22f9772c7e90b31a6064
src/tests/hierarchical_allocator_benchmarks.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/68548/diff/9/
Changes: https://reviews.apache.org/r/68548/diff/8-9/
Testing
-------
Thanks,
Kapil Arya
Re: Review Request 68548: Introduced a base class for writing
allocator benchmarks.
Posted by Kapil Arya <ka...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68548/
-----------------------------------------------------------
(Updated Sept. 14, 2018, 2 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 (updated)
-----
src/Makefile.am 0a9eeabe52b447a311de22f9772c7e90b31a6064
src/tests/hierarchical_allocator_benchmarks.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/68548/diff/8/
Changes: https://reviews.apache.org/r/68548/diff/7-8/
Testing
-------
Thanks,
Kapil Arya
Re: Review Request 68548: Introduced a base class for writing
allocator benchmarks.
Posted by Kapil Arya <ka...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68548/
-----------------------------------------------------------
(Updated Sept. 13, 2018, 7:04 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 (updated)
-----
src/Makefile.am 0a9eeabe52b447a311de22f9772c7e90b31a6064
src/tests/hierarchical_allocator_benchmarks.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/68548/diff/7/
Changes: https://reviews.apache.org/r/68548/diff/6-7/
Testing
-------
Thanks,
Kapil Arya
Re: Review Request 68548: Introduced a base class for writing
allocator benchmarks.
Posted by Kapil Arya <ka...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68548/
-----------------------------------------------------------
(Updated Sept. 13, 2018, 4:22 p.m.)
Review request for mesos, Meng Zhu and Till Toenshoff.
Changes
-------
Addressed Meng's comments
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 (updated)
-----
src/Makefile.am 0a9eeabe52b447a311de22f9772c7e90b31a6064
src/tests/hierarchical_allocator_benchmarks.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/68548/diff/6/
Changes: https://reviews.apache.org/r/68548/diff/5-6/
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. 11, 2018, 8:14 p.m., Meng Zhu wrote:
> > src/tests/hierarchical_allocator_benchmarks.cpp
> > Lines 250 (patched)
> > <https://reviews.apache.org/r/68548/diff/5/?file=2088299#file2088299line250>
> >
> > While it does not make a difference, it seems odd to pause the allocator here. Let's pause it immediately after the initialization.
Allocator can't add agent while paused :/ https://github.com/apache/mesos/blob/91ca8e2b2071f7e4b89702ae7c807b074bdef31b/src/master/allocator/mesos/hierarchical.cpp#L534
- Kapil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68548/#review208530
-----------------------------------------------------------
On Sept. 13, 2018, 4:22 p.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68548/
> -----------------------------------------------------------
>
> (Updated Sept. 13, 2018, 4:22 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/6/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Kapil Arya
>
>
Re: Review Request 68548: Introduced a base class for writing
allocator benchmarks.
Posted by Meng Zhu <mz...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68548/#review208530
-----------------------------------------------------------
src/tests/hierarchical_allocator_benchmarks.cpp
Lines 71-82 (patched)
<https://reviews.apache.org/r/68548/#comment292519>
Nits: prepending underscore for disambiguation in the constructor.
https://github.com/apache/mesos/blob/master/docs/c++-style-guide.md#variable-names
ditto below.
src/tests/hierarchical_allocator_benchmarks.cpp
Lines 86-87 (patched)
<https://reviews.apache.org/r/68548/#comment292521>
This is convoluted. I can only understand the intention of this by looking at the other test patch.
Let's remove this field and make instances of each `FrameworkProfile` share the same role (whether only one role or multi-roles). If the tester wants to launch 40 frameworks under different roles, then they should have 40 different `FrameworkProfiles`.
src/tests/hierarchical_allocator_benchmarks.cpp
Lines 144-155 (patched)
<https://reviews.apache.org/r/68548/#comment292542>
OK, I gave some thoughts about what should go into the `Benchmarkconfig` and what should just leave to the test body.
I think we should put whatever parameters the test harness currently consumes into the `Benchmarkconfig`, the rest should go to the test body. That means we should move `maxTasksPerOffer` and `testTimeout` to the individual test body.
Here is the rational. Ideally, the test writer does not need to know about the internals of the test harness instead, they customize `Benchmarkconfig` to configure it. So if a parameter is not consumed (i.e. honored) by the test harness, it should not be in `Benchmarkconfig`.
Of course, the long-term goal is to make `Benchmarkconfig` a one-stop configuration to both initialize and run the tests. But we are not there yet.
src/tests/hierarchical_allocator_benchmarks.cpp
Lines 250 (patched)
<https://reviews.apache.org/r/68548/#comment292524>
While it does not make a difference, it seems odd to pause the allocator here. Let's pause it immediately after the initialization.
src/tests/hierarchical_allocator_benchmarks.cpp
Lines 286-293 (patched)
<https://reviews.apache.org/r/68548/#comment292539>
I am not sure if all the tests will care about this. I think it makes sense for the test in your next patch. Let's move it there.
src/tests/hierarchical_allocator_benchmarks.cpp
Lines 306 (patched)
<https://reviews.apache.org/r/68548/#comment292525>
No allocator resume?
src/tests/hierarchical_allocator_benchmarks.cpp
Lines 311-314 (patched)
<https://reviews.apache.org/r/68548/#comment292540>
ditto, this may not make sense to all tests, let's not put it here.
- Meng Zhu
On Sept. 11, 2018, 12: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, 12: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>.
-----------------------------------------------------------
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.
Changes
-------
Addressed Meng's comments.
Summary (updated)
-----------------
Introduced a base class for writing allocator benchmarks.
Bugs: MESOS-9187
https://issues.apache.org/jira/browse/MESOS-9187
Repository: mesos
Description (updated)
-------
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 (updated)
-----
src/Makefile.am 0a9eeabe52b447a311de22f9772c7e90b31a6064
src/tests/hierarchical_allocator_benchmarks.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/68548/diff/5/
Changes: https://reviews.apache.org/r/68548/diff/4-5/
Testing
-------
Thanks,
Kapil Arya
Re: Review Request 68548: Added allocator benchmark test harness.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68548/#review208067
-----------------------------------------------------------
PASS: Mesos patch 68548 was successfully built and tested.
Reviews applied: `['68549', '68548']`
All the build artifacts available at: http://dcos-win.westus.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2253/mesos-review-68548
- Mesos Reviewbot Windows
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/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Kapil Arya
>
>
Re: Review Request 68548: Added allocator benchmark test harness.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68548/#review208203
-----------------------------------------------------------
PASS: Mesos patch 68548 was successfully built and tested.
Reviews applied: `['68549', '68548']`
All the build artifacts available at: http://dcos-win.westus.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2269/mesos-review-68548
- Mesos Reviewbot Windows
On Aug. 28, 2018, 7: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, 7: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/3/
>
>
> 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
>
>
Re: Review Request 68548: Added allocator benchmark test harness.
Posted by Meng Zhu <mz...@mesosphere.io>.
-----------------------------------------------------------
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: Added allocator benchmark test harness.
Posted by Meng Zhu <mz...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68548/#review208063
-----------------------------------------------------------
Glad to see this being added! Thank you!
I did a pass on the test harness. Will look into the test after the harness is updated.
src/tests/hierarchical_allocator_benchmarks.cpp
Lines 58 (patched)
<https://reviews.apache.org/r/68548/#comment291828>
Do we need this? Can you audit other "using/include" and remove any that are not needed?
src/tests/hierarchical_allocator_benchmarks.cpp
Lines 82 (patched)
<https://reviews.apache.org/r/68548/#comment291819>
`set<string>` would enable us to benchmark multi-role frameworks.
src/tests/hierarchical_allocator_benchmarks.cpp
Lines 85 (patched)
<https://reviews.apache.org/r/68548/#comment291807>
Take reference?
src/tests/hierarchical_allocator_benchmarks.cpp
Lines 91 (patched)
<https://reviews.apache.org/r/68548/#comment291806>
We can use `CHECK_NOTERROR` instead of the unguarded `get`.
Moreover, I think it is more flexible to just take a `Resource` object here, leaving the parsing or whatever to the caller. For non-trivial resource objects e.g. involving persistent volume, they are more likely to already be resource objects instead of a plain string.
src/tests/hierarchical_allocator_benchmarks.cpp
Lines 109 (patched)
<https://reviews.apache.org/r/68548/#comment291809>
ditto resources object.
src/tests/hierarchical_allocator_benchmarks.cpp
Lines 135-136 (patched)
<https://reviews.apache.org/r/68548/#comment291818>
Comparing to the original test harness, we probably want to control more things e.g. random allocator and other flag values in the future.
I suggest selectively keep/expose several fields such as interval, sorter type and etc. explicitly instead of keeping a default master flag. See my comments regarding `initializeCluster` below.
src/tests/hierarchical_allocator_benchmarks.cpp
Lines 155 (patched)
<https://reviews.apache.org/r/68548/#comment291829>
Are we relying on callers to name their frameworks ...%pid? Then this is very fragile. One solution is to just concatenate with `_#` and document it well. I think "nextFrameworkId" is used for that?
src/tests/hierarchical_allocator_benchmarks.cpp
Lines 160 (patched)
<https://reviews.apache.org/r/68548/#comment291830>
ditto
src/tests/hierarchical_allocator_benchmarks.cpp
Lines 162 (patched)
<https://reviews.apache.org/r/68548/#comment291831>
If we `push_back` last, we can std::move.
src/tests/hierarchical_allocator_benchmarks.cpp
Lines 167 (patched)
<https://reviews.apache.org/r/68548/#comment291820>
I would expect `frameworkTasksLaunched` tracks
the number of "launched tasks", not "tasks to launch".
Can you also add some comments to the fields whose meaning is not immediately obvious?
src/tests/hierarchical_allocator_benchmarks.cpp
Lines 183 (patched)
<https://reviews.apache.org/r/68548/#comment291832>
ditto
src/tests/hierarchical_allocator_benchmarks.cpp
Lines 190 (patched)
<https://reviews.apache.org/r/68548/#comment291821>
We can std::move here, even in the test, savings are savings:)
src/tests/hierarchical_allocator_benchmarks.cpp
Lines 197 (patched)
<https://reviews.apache.org/r/68548/#comment291824>
I think this TODO is no longer relavent.
src/tests/hierarchical_allocator_benchmarks.cpp
Lines 199-200 (patched)
<https://reviews.apache.org/r/68548/#comment291822>
As mentioned above, let's expose more knobs here. For now, maybe just allocator type, allocation interval, offercallback and any other that are currently being touched.
This will likely make the initialize function signature ugly. It is probably better to introduce a flag (maybe utilize the `BenchmarkConfig` below?). You probably can come up with a better solution.
src/tests/hierarchical_allocator_benchmarks.cpp
Lines 220 (patched)
<https://reviews.apache.org/r/68548/#comment291825>
ah, one thing I forget to mention in the original patch. This vector here is fragile. It will be concurrently accessed by both allocator actor and the test actor. Like the test base, let's use an asynchronous queue here.
https://github.com/apache/mesos/blob/master/src/tests/hierarchical_allocator_tests.cpp#L282
src/tests/hierarchical_allocator_benchmarks.cpp
Lines 241 (patched)
<https://reviews.apache.org/r/68548/#comment291823>
Create frameworks and agents. (Or just remove the comment).
src/tests/hierarchical_allocator_benchmarks.cpp
Lines 345-346 (patched)
<https://reviews.apache.org/r/68548/#comment291826>
The meaning of these fields are not obvious, adding some comments would be helpful.
src/tests/hierarchical_allocator_benchmarks.cpp
Lines 350 (patched)
<https://reviews.apache.org/r/68548/#comment291827>
hmm, just to understand the test configuration, I will need to go through a class inheritance, not ideal.
I think we need to differentiate benchmark "configuration" and "parameters". Configuration can just go into the test body (mostly about setting the "flag" I mentioned above). We can optionally parameterized the benchmark using the gtest parameter interface.
src/tests/hierarchical_allocator_benchmarks.cpp
Lines 390 (patched)
<https://reviews.apache.org/r/68548/#comment291834>
As mentioned above, there seems to be no need to paramtereized the benchmark (in its current form) here.
However, I think we should add parameters like number of agents and frameworks.
- 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/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Kapil Arya
>
>