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/09/11 19:15:05 UTC
Re: Review Request 68548: Introduced a base class for writing
allocator benchmarks.
-----------------------------------------------------------
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: 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
>
>