Add auto-tuning of max executor and hash partition #18716

vermapratyush · 2022-11-22T18:55:42Z

Test Plan

Added unit test to the classes modified.
Ran presto server on local and submitted queries on "customers" table to confirm correct hash_partition_count is used.

Summary

Added 2 configuration options (spark_executor_allocation_strategy_enabled, spark_hash_partition_count_allocation_strategy_enabled) to auto-tune, spark.dynamicAllocation.maxExecutors and hash_partition_count based on the input table stats, respectively.

The auto tune configuration option take precedence over explicitly provided values for spark.dynamicAllocation.maxExecutors and hash_partition_count.

If spark_resource_allocation_strategy_enabled is enabled, both executor and hash partition are considered to be auto-tune enabled. This configuration option could be removed in subsequent revision, as it is redundant.

== RELEASE NOTES ==

General Changes
* Add property ``spark_executor_allocation_strategy_enabled`` to auto-tune spark max executor count (``spark.dynamicAllocation.maxExecutors``)  based on input data. Only required if ``spark_resource_allocation_strategy_enabled`` is not already enabled.
* Add property ``spark_hash_partition_count_allocation_strategy_enabled`` to auto-tune hash partition count (``hash_partition_count``) based on input data. Only required if ``spark_resource_allocation_strategy_enabled`` is not already enabled.

linux-foundation-easycla · 2022-11-22T18:55:45Z

The committers listed above are authorized under a signed CLA.

✅ login: vermapratyush / name: Pratyush Verma (1ca1e3296362bb6a3b977061034fd96af0843237)

v-jizhang · 2022-11-23T18:10:52Z

@bot kick off tests

vermapratyush · 2022-11-23T19:24:17Z

@bot kick off tests

...park-base/src/main/java/com/facebook/presto/spark/PrestoSparkPhysicalResourceCalculator.java

...-base/src/main/java/com/facebook/presto/spark/execution/PrestoSparkStaticQueryExecution.java

ajaygeorge · 2022-12-06T20:01:10Z

@vermapratyush Can you please sign the CLA

ajaygeorge · 2022-12-06T20:02:14Z

@vermapratyush Please squash the commits as well.

vermapratyush · 2022-12-06T21:01:22Z

@vermapratyush Can you please sign the CLA

@ajaygeorge Already signed the CLA, but somehow it still keeps failing.

ajaygeorge · 2022-12-06T21:04:32Z

@vermapratyush Can you please sign the CLA

@ajaygeorge Already signed the CLA, but somehow it still keeps failing.

Can you try signing the individual contributor CLA as well if not already.

ajaygeorge

Added some comments

presto-spark-base/src/main/java/com/facebook/presto/spark/PhysicalResourceSettingsHolder.java

presto-spark-base/src/main/java/com/facebook/presto/spark/PrestoSparkConfig.java

...park-base/src/main/java/com/facebook/presto/spark/PrestoSparkPhysicalResourceCalculator.java

vermapratyush · 2022-12-08T01:56:03Z

Unrelated failure in module presto-cache.
Re-ran the test locally and was successful.

...park-base/src/main/java/com/facebook/presto/spark/PrestoSparkPhysicalResourceCalculator.java

presto-spark-base/src/main/java/com/facebook/presto/spark/PhysicalResourceSettings.java

presto-spark-base/src/main/java/com/facebook/presto/spark/PrestoSparkConfig.java

pgupta2 · 2022-12-10T04:23:58Z

...park-base/src/main/java/com/facebook/presto/spark/PrestoSparkPhysicalResourceCalculator.java


-        double inputDataInBytes = new PrestoSparkSourceStatsCollector(metaData, session).collectSourceStats(plan);
+        if (!anyAllocationStrategyEnabled(session)) {


We can do the check for the main property here: isSparkResourceAllocationStrategyEnabled(session) and if its disabled, we return back. This is basically a single switch to disable allocation strategy completely.

Basically, my thinking is that spark_resource_allocation_strategy_enabled control this whole logic. If this is disabled, we skip this logic completely. If spark_resource_allocation_strategy_enabled is true, then only we check for individual strategy properties and trigger the one that is enabled.

I know this is bit weird. Ideally, we should just remove spark_resource_allocation_strategy_enabled session property to make this logic cleaner

@highker : What is the right approach of deprecating/removing a property?

What is the right approach of deprecating/removing a property?

We use @DefunctConfig annotation in a config class. Usually, we also rename the property to deprecated.XXX. Check FeatureConfig for examples. We don't usually delete the property directly.

We have release note to tell users what config has the deprecated one migrated to

pgupta2 · 2022-12-10T04:24:43Z

...park-base/src/main/java/com/facebook/presto/spark/PrestoSparkPhysicalResourceCalculator.java

+            return defaultResourceSettings;
+        }
+        // update hashPartitionCount only if resource allocation or hash partition allocation is enabled
+        if (isSparkResourceAllocationStrategyEnabled(session) || isSparkHashPartitionCountAllocationStrategyEnabled(session)) {


Check for isSparkResourceAllocationStrategyEnabled(session) can be removed here.

Will deprecate this config in subsequent PR.

Should we make it an AND condition? That will give us the ability to control individual optimizations separately. Else if isSparkResourceAllocationStrategyEnabled(session) is true, both optimizations will run regardless.

pgupta2 · 2022-12-10T04:24:52Z

...park-base/src/main/java/com/facebook/presto/spark/PrestoSparkPhysicalResourceCalculator.java

-            log.warn(String.format("Failed to retrieve correct size, data read=%.2f, skipping automatic resource tuning.", inputDataInBytes));
-            return DISABLED_PHYSICAL_RESOURCE_SETTING;
+        // update maxExecutorCount only if resource allocation or executor allocation is enabled
+        if (isSparkResourceAllocationStrategyEnabled(session) || isSparkExecutorAllocationStrategyEnabled(session)) {


Same as above comment.

Will deprecate this config in subsequent PR.

presto-spark-base/src/main/java/com/facebook/presto/spark/planner/PrestoSparkQueryPlanner.java

ajaygeorge

LGTM % a few nits.

presto-spark-base/src/main/java/com/facebook/presto/spark/PhysicalResourceSettings.java

presto-spark-base/src/main/java/com/facebook/presto/spark/PrestoSparkConfig.java

pgupta2

Just 1 final comment.

pgupta2 · 2022-12-16T01:47:27Z

...park-base/src/main/java/com/facebook/presto/spark/PrestoSparkPhysicalResourceCalculator.java

+            return defaultResourceSettings;
+        }
+        // update hashPartitionCount only if resource allocation or hash partition allocation is enabled
+        if (isSparkResourceAllocationStrategyEnabled(session) || isSparkHashPartitionCountAllocationStrategyEnabled(session)) {


Should we make it an AND condition? That will give us the ability to control individual optimizations separately. Else if isSparkResourceAllocationStrategyEnabled(session) is true, both optimizations will run regardless.

vermapratyush · 2022-12-16T09:45:09Z

@pgupta2 I don't want to couple isSparkResourceAllocationStrategyEnabled and isSparkHashPartitionCountAllocationStrategyEnabled together since isSparkResourceAllocationStrategyEnabled will be going away in subsequent PR anyway.

presto-spark-base/src/main/java/com/facebook/presto/spark/PhysicalResourceSettings.java

...ava/com/facebook/presto/spark/planner/TestPrestoSparkPhysicalResourceAllocationStrategy.java

vermapratyush marked this pull request as ready for review November 22, 2022 23:03

vermapratyush requested a review from a team as a code owner November 22, 2022 23:03

vermapratyush requested a review from presto-oss November 22, 2022 23:03

pgupta2 requested changes Dec 1, 2022

View reviewed changes

vermapratyush force-pushed the exec-hash-alloc-prpty-1 branch from 8c076fd to d88f57f Compare December 5, 2022 16:33

vermapratyush requested review from pgupta2 and removed request for presto-oss December 6, 2022 15:17

vermapratyush force-pushed the exec-hash-alloc-prpty-1 branch from 15587ef to 3197e4a Compare December 6, 2022 21:00

vermapratyush force-pushed the exec-hash-alloc-prpty-1 branch 2 times, most recently from 1ca1e32 to db890d3 Compare December 6, 2022 22:27

ajaygeorge reviewed Dec 7, 2022

View reviewed changes

vermapratyush force-pushed the exec-hash-alloc-prpty-1 branch 2 times, most recently from e410779 to eb5f715 Compare December 8, 2022 01:23

vermapratyush requested review from ajaygeorge and pgupta2 and removed request for pgupta2 and ajaygeorge December 8, 2022 01:55

ajaygeorge reviewed Dec 9, 2022

View reviewed changes

...park-base/src/main/java/com/facebook/presto/spark/PrestoSparkPhysicalResourceCalculator.java Outdated Show resolved Hide resolved

vermapratyush force-pushed the exec-hash-alloc-prpty-1 branch from eb5f715 to 45c214d Compare December 9, 2022 11:50

pgupta2 reviewed Dec 12, 2022

View reviewed changes

ajaygeorge approved these changes Dec 12, 2022

View reviewed changes

vermapratyush mentioned this pull request Dec 12, 2022

Deprecate spark_resource_allocation_strategy_enabled property #18801

Closed

vermapratyush force-pushed the exec-hash-alloc-prpty-1 branch 3 times, most recently from 8fa0604 to d1de7d5 Compare December 15, 2022 14:03

pgupta2 approved these changes Dec 16, 2022

View reviewed changes

vermapratyush force-pushed the exec-hash-alloc-prpty-1 branch from d1de7d5 to fd34c8c Compare December 16, 2022 16:40

vermapratyush requested a review from highker December 16, 2022 17:58

highker approved these changes Dec 16, 2022

View reviewed changes

vermapratyush force-pushed the exec-hash-alloc-prpty-1 branch 2 times, most recently from 4df8b74 to 6a7b385 Compare December 16, 2022 18:59

Add auto-tuning of max executor and hash partition

3ce7856

vermapratyush force-pushed the exec-hash-alloc-prpty-1 branch from 6a7b385 to 3ce7856 Compare December 16, 2022 19:48

rschlussel merged commit ddca5d1 into prestodb:master Dec 19, 2022

wanglinsong mentioned this pull request Jan 12, 2023

Add release notes for 0.279 #18920

Merged

30 tasks

vermapratyush deleted the exec-hash-alloc-prpty-1 branch April 7, 2023 20:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add auto-tuning of max executor and hash partition #18716

Add auto-tuning of max executor and hash partition #18716

vermapratyush commented Nov 22, 2022 •

edited

Loading

linux-foundation-easycla bot commented Nov 22, 2022 •

edited

Loading

v-jizhang commented Nov 23, 2022

vermapratyush commented Nov 23, 2022

ajaygeorge commented Dec 6, 2022

ajaygeorge commented Dec 6, 2022

vermapratyush commented Dec 6, 2022

ajaygeorge commented Dec 6, 2022

ajaygeorge left a comment

vermapratyush commented Dec 8, 2022

pgupta2 Dec 10, 2022

highker Dec 12, 2022

pgupta2 Dec 10, 2022

vermapratyush Dec 12, 2022

pgupta2 Dec 16, 2022 •

edited

Loading

pgupta2 Dec 10, 2022

vermapratyush Dec 12, 2022

ajaygeorge left a comment

pgupta2 left a comment

pgupta2 Dec 16, 2022 •

edited

Loading

vermapratyush commented Dec 16, 2022


		double inputDataInBytes = new PrestoSparkSourceStatsCollector(metaData, session).collectSourceStats(plan);
		if (!anyAllocationStrategyEnabled(session)) {

Add auto-tuning of max executor and hash partition #18716

Add auto-tuning of max executor and hash partition #18716

Conversation

vermapratyush commented Nov 22, 2022 • edited Loading

Test Plan

Summary

linux-foundation-easycla bot commented Nov 22, 2022 • edited Loading

v-jizhang commented Nov 23, 2022

vermapratyush commented Nov 23, 2022

ajaygeorge commented Dec 6, 2022

ajaygeorge commented Dec 6, 2022

vermapratyush commented Dec 6, 2022

ajaygeorge commented Dec 6, 2022

ajaygeorge left a comment

Choose a reason for hiding this comment

vermapratyush commented Dec 8, 2022

pgupta2 Dec 10, 2022

Choose a reason for hiding this comment

highker Dec 12, 2022

Choose a reason for hiding this comment

pgupta2 Dec 10, 2022

Choose a reason for hiding this comment

vermapratyush Dec 12, 2022

Choose a reason for hiding this comment

pgupta2 Dec 16, 2022 • edited Loading

Choose a reason for hiding this comment

pgupta2 Dec 10, 2022

Choose a reason for hiding this comment

vermapratyush Dec 12, 2022

Choose a reason for hiding this comment

ajaygeorge left a comment

Choose a reason for hiding this comment

pgupta2 left a comment

Choose a reason for hiding this comment

pgupta2 Dec 16, 2022 • edited Loading

Choose a reason for hiding this comment

vermapratyush commented Dec 16, 2022

vermapratyush commented Nov 22, 2022 •

edited

Loading

linux-foundation-easycla bot commented Nov 22, 2022 •

edited

Loading

pgupta2 Dec 16, 2022 •

edited

Loading

pgupta2 Dec 16, 2022 •

edited

Loading