Add session property to enforce timeout for query registration in HBO by feilong-liu · Pull Request #23354 · prestodb/presto

feilong-liu · 2024-08-01T17:46:41Z

Description

HBO optimizer has several external dependency, it retrieves history statistics from external storage, get input table size information from metastore. It also canonicalize and hash the whole query plan, which can also be expensive when the query plan is large.
While latency of HBO optimizer is generally low, we have observed high latency for some queries occasionally. This has prohibited the usage of HBO optimizer in interactive workload with high latency requirement. Since HBO is an opportunistic optimization, i.e. queries still work without HBO, we can choose to skip it if it takes a long time to complete.
In this PR I add a session property to enforce timeout so that we can cap the maximum time spent in HBO optimizer

Motivation and Context

To enable HBO for interactive workload with high latency requirement

Impact

Enable HBO for interactive workload

Test Plan

Run end to end with production traffic, and also add unit tests

Contributor checklist

Please make sure your submission complies with our development, formatting, commit message, and attribution guidelines.
PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
Documented new properties (with its default value), SQL syntax, functions, or other functionality.
If release notes are required, they follow the release notes guidelines.
Adequate tests were added if applicable.
CI passed.

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== RELEASE NOTES ==

General Changes
* Add session property `enforce_history_based_optimizer_register_timeout` to enforce the maximum time HBO query registration can take :pr:`23354 `

feilong-liu · 2024-08-01T17:48:35Z

...ook/presto/sql/planner/optimizations/HistoricalStatisticsEquivalentPlanMarkingOptimizer.java

+        boolean registerSucceed = false;
+        if (enforceHistoryBasedOptimizerRegistrationTimeout(session)) {
+            ExecutorService executor = Executors.newSingleThreadExecutor();
+            Future<Boolean> future = executor.submit(() -> statsCalculator.registerPlan(newPlan, session, startTimeInNano, timeoutInMilliseconds));
+            try {
+                registerSucceed = future.get(timeoutInMilliseconds, MILLISECONDS);
+            }
+            catch (Exception ignored) {
+            }
+            finally {
+                executor.shutdownNow();
+            }


This is the main change

steveburnett · 2024-08-01T17:53:31Z

Suggest documenting the session property in https://prestodb.io/docs/current/optimizer/history-based-optimization.html#how-to-use-hbo
Nit revision suggestion for the release note entry for consistency with the Release Notes Guidelines:

== RELEASE NOTES ==

General Changes
* Add session property `enforce_history_based_optimizer_register_timeout` to enforce the maximum time HBO query registration can take :pr:`23354 `

steveburnett

Thanks for adding the doc!

The config properties table is malformed, throwing this error in a local doc build:

/Users/steveburnett/Documents/GitHub/presto/presto-docs/src/main/sphinx/optimizer/history-based-optimization.rst:34: ERROR: Malformed table. Column span alignment problem in table line 3.

See my comment in PR #23335 and my own PR #23322 for how to fix the table.

feilong-liu · 2024-08-01T19:21:15Z

Thanks for adding the doc!

The config properties table is malformed, throwing this error in a local doc build:

/Users/steveburnett/Documents/GitHub/presto/presto-docs/src/main/sphinx/optimizer/history-based-optimization.rst:34: ERROR: Malformed table. Column span alignment problem in table line 3.

See my comment in PR #23335 and my own PR #23322 for how to fix the table.

For the two PRs referenced above, I see imbalance between line 32 and line34. However in my case these two lines match. Is there any fix which can be done here?

steveburnett · 2024-08-01T19:40:20Z

For the two PRs referenced above, I see imbalance between line 32 and line34. However in my case these two lines match. Is there any fix which can be done here?

I apologize, I must have been on a wrong branch from another PR. I have restarted my review confirming that I am working with your branch in this PR and you are correct, you don't have this error. Please ignore my previous comment.

steveburnett

LGTM! (docs)

Pulled this branch, made a new local doc build, and the new property entries in both tables look good. Thanks!

...ook/presto/sql/planner/optimizations/HistoricalStatisticsEquivalentPlanMarkingOptimizer.java

rschlussel · 2024-08-01T20:07:11Z

presto-tests/src/test/java/com/facebook/presto/execution/TestHistoryBasedStatsTracking.java

    }

+    @Test
+    public void testHistoryBasedStatsCalculatorEnforceTimeOut()


this looks like it tests cases where the timeout is enabled, but never exceeded. Can you add a test case where the timeout gets enforced, eg. you set the timeout really low (like to 0?) and then check that we don't use hbo stats?

Added test with time limit to be 0

rschlussel · 2024-08-01T21:41:28Z

presto-tests/src/test/java/com/facebook/presto/execution/TestHistoryBasedStatsTracking.java

+                "SELECT * FROM nation where substr(name, 1, 1) = 'A'",
+                anyTree(node(FilterNode.class, any()).withOutputRowCount(Double.NaN)));
+
+        // HBO Statistics


why is this using hbo statistics? Shouldn't registration fail due to exceeding the time limit?

This is to test the case where HBO statistics are written successfully (with large timeout limit), and later it tests that read of HBO statistics either succeed (if timeout limit is large) or fail (if timeout limit is 0).

I just added a few new test cases too, which is HBO statistics write failed (with 0 timeout limit), and no HBO statistics will be read.

…imizer

feilong-liu · 2024-08-01T22:22:21Z

...in/src/main/java/com/facebook/presto/testing/InMemoryHistoryBasedPlanStatisticsProvider.java

+    public void noProcessQueryEvents()
+    {
+        try {
+            assertFalse(semaphore.tryAcquire(10, TimeUnit.SECONDS));


semaphore is released during the putStats function call. If HBO failed, there will be no putStats function call, hence we will not get the semaphore here. Use this function to assert the case where HBO timeout

feilong-liu requested review from a team, elharo, jaystarshot and vivek-bharathan as code owners August 1, 2024 17:46

feilong-liu requested a review from presto-oss August 1, 2024 17:46

feilong-liu marked this pull request as draft August 1, 2024 17:46

feilong-liu commented Aug 1, 2024

View reviewed changes

feilong-liu force-pushed the hbo_timeout branch from 1597d5f to 0934faa Compare August 1, 2024 18:41

feilong-liu requested a review from rschlussel August 1, 2024 18:41

feilong-liu marked this pull request as ready for review August 1, 2024 18:53

feilong-liu requested a review from steveburnett as a code owner August 1, 2024 18:53

feilong-liu force-pushed the hbo_timeout branch from 0934faa to ce982f2 Compare August 1, 2024 18:56

steveburnett requested changes Aug 1, 2024

View reviewed changes

feilong-liu requested a review from steveburnett August 1, 2024 19:21

steveburnett previously approved these changes Aug 1, 2024

View reviewed changes

rschlussel reviewed Aug 1, 2024

View reviewed changes

feilong-liu dismissed steveburnett’s stale review via 62b9c91 August 1, 2024 21:21

feilong-liu force-pushed the hbo_timeout branch from ce982f2 to 62b9c91 Compare August 1, 2024 21:21

feilong-liu requested a review from rschlussel August 1, 2024 21:22

rschlussel reviewed Aug 1, 2024

View reviewed changes

Add session property to enforce timeout for query register in HBO opt…

bf91cdb

…imizer

feilong-liu force-pushed the hbo_timeout branch from 62b9c91 to bf91cdb Compare August 1, 2024 22:18

feilong-liu commented Aug 1, 2024

View reviewed changes

feilong-liu requested review from rschlussel and removed request for rschlussel August 1, 2024 22:22

rschlussel approved these changes Aug 7, 2024

View reviewed changes

feilong-liu merged commit 39e74cc into prestodb:master Aug 7, 2024

feilong-liu deleted the hbo_timeout branch August 7, 2024 21:00

tdcmeehan mentioned this pull request Aug 23, 2024

Add release notes for 0.289 #23513

Merged

34 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add session property to enforce timeout for query registration in HBO#23354

Add session property to enforce timeout for query registration in HBO#23354
feilong-liu merged 1 commit intoprestodb:masterfrom
feilong-liu:hbo_timeout

feilong-liu commented Aug 1, 2024 •

edited

Loading

Uh oh!

feilong-liu Aug 1, 2024

Uh oh!

steveburnett commented Aug 1, 2024

Uh oh!

steveburnett left a comment •

edited

Loading

Uh oh!

feilong-liu commented Aug 1, 2024

Uh oh!

steveburnett commented Aug 1, 2024

Uh oh!

steveburnett left a comment

Uh oh!

Uh oh!

rschlussel Aug 1, 2024

Uh oh!

feilong-liu Aug 1, 2024

Uh oh!

rschlussel Aug 1, 2024

Uh oh!

feilong-liu Aug 1, 2024

Uh oh!

feilong-liu Aug 1, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

feilong-liu commented Aug 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

Impact

Test Plan

Contributor checklist

Release Notes

Uh oh!

feilong-liu Aug 1, 2024

Choose a reason for hiding this comment

Uh oh!

steveburnett commented Aug 1, 2024

Uh oh!

steveburnett left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

feilong-liu commented Aug 1, 2024

Uh oh!

steveburnett commented Aug 1, 2024

Uh oh!

steveburnett left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rschlussel Aug 1, 2024

Choose a reason for hiding this comment

Uh oh!

feilong-liu Aug 1, 2024

Choose a reason for hiding this comment

Uh oh!

rschlussel Aug 1, 2024

Choose a reason for hiding this comment

Uh oh!

feilong-liu Aug 1, 2024

Choose a reason for hiding this comment

Uh oh!

feilong-liu Aug 1, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

feilong-liu commented Aug 1, 2024 •

edited

Loading

steveburnett left a comment •

edited

Loading