BenchmarkRunner fixes and make it extensible by caithagoras · Pull Request #14670 · prestodb/presto

caithagoras · 2020-06-18T01:47:20Z

== NO RELEASE NOTES ==

yingsu00 · 2020-06-18T03:04:16Z

Make BenchmarkSupplierSupplier extensible Did you mean BenchmarkSuiteSupplier instead?

caithagoras · 2020-06-18T03:14:00Z

@yingsu00 Yes. Updated all commit messages.

yingsu00

Make BenchmarkSuiteSupplier extensible LGTM with a tiny nit
Make benchmark runner extensible LGTM
Run out of time today. Will continue read tomorrow

...nner/src/test/java/com/facebook/presto/benchmark/source/TestMySqlBenchmarkSuiteSupplier.java

caithagoras · 2020-06-18T19:53:59Z

@yingsu00 Thanks. comments addressed.

caithagoras · 2020-06-19T06:10:35Z

Added a few more refactoring commits to prepare for supporting StreamPhaseExecutor. Adding tests for commit 6.

caithagoras · 2020-06-19T22:45:09Z

@yingsu00 All tests added.

yingsu00

Make max concurrency optional in ConcurrentExecutionPhase LGTM. Just add comment to define what STREAM/CONCURRENT means.

yingsu00 · 2020-06-26T01:00:10Z

...enchmark-runner/src/main/java/com/facebook/presto/benchmark/framework/ExecutionStrategy.java

Does STREAM mean single stream and CONCURRENT means multiple of streams? Or CONCURRENT means no streams? Could you add some comment?

yingsu00

Fix PhaseSpecification LGTM

yingsu00 · 2020-06-26T03:51:34Z

...o-benchmark-runner/src/main/java/com/facebook/presto/benchmark/framework/BenchmarkSuite.java

Why is the queries mutable? Everything else is final, why does the queries need to change given a fixed suite and querySet?

Answered below.

yingsu00 · 2020-06-26T03:53:45Z

...r/src/main/java/com/facebook/presto/benchmark/source/AbstractJdbiBenchmarkSuiteSupplier.java

Same question as above: why do we need to use setQueries instead of make it part of the constructor?

You can find that all fields of BenchmarkSuite are immutable. queries is mutable only in this JdbiBuilder inner-class.

Why do we need to use setQueries instead of make it part of the constructor?
This is because we need to run 2 MySQL queries to fetch all information about a suite.

dao.getBenchmarkSuite gets some the suite configuration by reading benchmark_suites.

dao.getBenchmarkQueries gets the list of queries by reading benchmark_queries.

The result of 1st query needs to be stored in some intermediate object. Before the refactoring, the result is mapped to BenchmarkSuiteInfo class, and a BenchmarkSuite contains both BenchmarkSuiteInfo and the query lists. With the refactoring, I removed BenchmarkSuiteInfo, and replace it with the builder class instead. The result of the 1st query is mapped to BenchmarkSuite.JdbiBuilder using the constructor, while the result of the 2nd query is added to the build using the setter. And finally JdbiBuilder.build() returns a BenchmarkSuite object.

yingsu00 · 2020-06-26T03:58:04Z

presto-benchmark-runner/src/test/java/com/facebook/presto/benchmark/BenchmarkTestUtil.java

Why are all tests with empty session properties? Can we add some test with non-empty session properties?

Test added.

yingsu00 · 2020-06-30T17:33:16Z

@caithagoras any thoughts on the comments?

- Move the logic of DbBenchmarkSuiteSupplier into an abastract base class AbstractJdbiBenchmarkSuiteSupplier. - Move MySqlBenchmarkSuiteConfig to the correct package.

Move logic from PrestoBenchmarkCommand to an abstract base class.

Fix JSON serialization and deserialization for subclasses of PhaseSpecification. It is unnecessay to make ExecutionStrategy a field or JSON field. Instead, ExecutionStrategy is static for any subclasses of PhaseExecution. Also, - Fix equals() and hashCode() - Move ExecutionStrategy to top-level

Remove BenchmarkSuiteInfo and move all its fields into BenchmarkSuite. Use a builder object of BenchmarkSuite to hold the intermediate suite information fetched from MySQL.

- Re-define interface PhaseExecutor so that a PhaseExecutor is a stateless object and is reused across phases. - Move BenchmarQueryRunner into AbstractPhaseExecutor. - Shutdown executor in a single place. - Post events in single places. - Move all classes in executor package to framework package.

Allow max concurrency for concurrent phases to be specified through config. - Use the max concurrency from config if specified. - Else, use the max concurrency specified in phase specification. - Else, use the default max concurrency 70.

caithagoras · 2020-06-30T20:47:10Z

@yingsu00 comments addressed

caithagoras · 2020-06-30T20:56:32Z

@mbasmanova Could you help approve this PR?

mbasmanova · 2020-06-30T21:18:11Z

@caithagoras I don't have context on this change and I won't have bandwidth to review it.

yingsu00 · 2020-06-30T21:43:25Z

@rongrong will you be able to review this PR? Thank you!

yingsu00 · 2020-06-30T21:47:33Z

@caithagoras I don't have context on this change and I won't have bandwidth to review it.

Hi Masha, this PR is part of Issue #14111

caithagoras · 2020-07-07T21:17:04Z

cc @rongrong for final approval

rongrong · 2020-07-08T21:43:25Z

...-benchmark-runner/src/main/java/com/facebook/presto/benchmark/framework/BenchmarkRunner.java

 {
    private final BenchmarkSuiteSupplier benchmarkSuiteSupplier;
-    private final PhaseExecutorFactory phaseExecutorFactory;
+    private final ConcurrentPhaseExecutor concurrentPhaseExecutor;


So basically BenchmarkRunner will have as many executors are there are execution strategies?

Yes, and there will only be 2 in the foreseeable future. CONCURRENT and STREAM

caithagoras requested a review from yingsu00 June 18, 2020 01:47

caithagoras force-pushed the s2 branch 2 times, most recently from c2b99c1 to 95eb1a0 Compare June 18, 2020 03:13

yingsu00 reviewed Jun 18, 2020

View reviewed changes

...nner/src/test/java/com/facebook/presto/benchmark/source/TestMySqlBenchmarkSuiteSupplier.java Outdated Show resolved Hide resolved

caithagoras force-pushed the s2 branch from 95eb1a0 to a3ad158 Compare June 18, 2020 19:53

caithagoras force-pushed the s2 branch 4 times, most recently from 10c70f4 to 7103f33 Compare June 19, 2020 06:09

caithagoras force-pushed the s2 branch 4 times, most recently from 6830665 to 96294e7 Compare June 19, 2020 22:44

yingsu00 reviewed Jun 26, 2020

View reviewed changes

yingsu00 requested a review from mbasmanova June 30, 2020 19:01

caithagoras0 added 2 commits June 30, 2020 13:04

Make BenchmarkSuiteSupplier extensible

bd91b16

- Move the logic of DbBenchmarkSuiteSupplier into an abastract base class AbstractJdbiBenchmarkSuiteSupplier. - Move MySqlBenchmarkSuiteConfig to the correct package.

Make benchmark runner extensible

94fad97

Move logic from PrestoBenchmarkCommand to an abstract base class.

caithagoras force-pushed the s2 branch from 96294e7 to 6df620b Compare June 30, 2020 20:04

caithagoras0 added 2 commits June 30, 2020 13:13

Refactor BenchmarkSuite

60147d7

Remove BenchmarkSuiteInfo and move all its fields into BenchmarkSuite. Use a builder object of BenchmarkSuite to hold the intermediate suite information fetched from MySQL.

caithagoras force-pushed the s2 branch from 6df620b to a1b8e30 Compare June 30, 2020 20:13

Allow session properties to be specified with queries

03be023

caithagoras0 added 2 commits June 30, 2020 13:45

caithagoras force-pushed the s2 branch from a1b8e30 to 1cf42b0 Compare June 30, 2020 20:46

yingsu00 self-requested a review June 30, 2020 21:05

yingsu00 approved these changes Jun 30, 2020

View reviewed changes

yingsu00 requested a review from rongrong June 30, 2020 21:42

rongrong approved these changes Jul 8, 2020

View reviewed changes

caithagoras merged commit 2cb629e into prestodb:master Jul 22, 2020

caithagoras deleted the s2 branch July 22, 2020 22:31

caithagoras mentioned this pull request Jul 28, 2020

Add release notes for 0.239 #14908

Merged

13 tasks

Conversation

caithagoras commented Jun 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yingsu00 commented Jun 18, 2020

Uh oh!

caithagoras commented Jun 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yingsu00 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

caithagoras commented Jun 18, 2020

Uh oh!

caithagoras commented Jun 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

caithagoras commented Jun 19, 2020

Uh oh!

yingsu00 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yingsu00 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yingsu00 commented Jun 30, 2020

Uh oh!

caithagoras commented Jun 30, 2020

Uh oh!

caithagoras commented Jun 30, 2020

Uh oh!

mbasmanova commented Jun 30, 2020

Uh oh!

yingsu00 commented Jun 30, 2020

Uh oh!

yingsu00 commented Jun 30, 2020

Uh oh!

caithagoras commented Jul 7, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

caithagoras commented Jun 18, 2020 •

edited

Loading

caithagoras commented Jun 18, 2020 •

edited

Loading

caithagoras commented Jun 19, 2020 •

edited

Loading