Support query retry for HBO#23147
Merged
abhinavmuk04 merged 1 commit intoprestodb:masterfrom Aug 2, 2024
abhinavmuk04:retryforhbo
Merged
Support query retry for HBO#23147abhinavmuk04 merged 1 commit intoprestodb:masterfrom abhinavmuk04:retryforhbo
abhinavmuk04 merged 1 commit intoprestodb:masterfrom
abhinavmuk04:retryforhbo
Conversation
Contributor
|
I understand this is draft, but what is the design or idea behind this change? |
Contributor
Author
The goal is to implement a simple version of retry query support for HBO in Presto, which retries failed queries and checks if the query plan has changed before re-executing. Currently, just building out basic portions. |
jaystarshot
reviewed
Jul 24, 2024
presto-main/src/main/java/com/facebook/presto/server/protocol/Query.java
Outdated
Show resolved
Hide resolved
elharo
reviewed
Jul 25, 2024
presto-tests/src/test/java/com/facebook/presto/tests/TestHistoryBasedRetry.java
Outdated
Show resolved
Hide resolved
presto-tests/src/test/java/com/facebook/presto/tests/TestHistoryBasedRetry.java
Outdated
Show resolved
Hide resolved
presto-tests/src/test/java/com/facebook/presto/tests/TestHistoryBasedRetry.java
Outdated
Show resolved
Hide resolved
presto-tests/src/test/java/com/facebook/presto/tests/TestHistoryBasedRetry.java
Outdated
Show resolved
Hide resolved
presto-tests/src/test/java/com/facebook/presto/tests/TestHistoryBasedRetry.java
Outdated
Show resolved
Hide resolved
presto-tests/src/test/java/com/facebook/presto/tests/TestHistoryBasedRetry.java
Outdated
Show resolved
Hide resolved
feilong-liu
reviewed
Jul 26, 2024
presto-main/src/main/java/com/facebook/presto/SystemSessionProperties.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/server/protocol/Query.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/server/protocol/Query.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/server/protocol/Query.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/server/protocol/Query.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/server/protocol/Query.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/server/protocol/Query.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/server/protocol/Query.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/server/protocol/Query.java
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/server/protocol/Query.java
Outdated
Show resolved
Hide resolved
Contributor
agrawaldevesh
left a comment
There was a problem hiding this comment.
I suggest updating the test plan in the PR description to include exactly how you verified e2e that a retry is happening for both cases where a retry should happen and where it shouldn't. Please give examples of queries.
presto-main/src/main/java/com/facebook/presto/server/protocol/Query.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/server/protocol/Query.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/server/protocol/Query.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/server/protocol/Query.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/server/protocol/Query.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/server/protocol/Query.java
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/server/protocol/Query.java
Outdated
Show resolved
Hide resolved
feilong-liu
reviewed
Jul 30, 2024
presto-main/src/main/java/com/facebook/presto/server/protocol/Query.java
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/server/protocol/Query.java
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/server/protocol/Query.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/server/protocol/Query.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/server/protocol/Query.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/server/protocol/Query.java
Outdated
Show resolved
Hide resolved
feilong-liu
reviewed
Jul 31, 2024
Contributor
feilong-liu
left a comment
There was a problem hiding this comment.
Can you also shadow production traffic with the feature turned on?
presto-main/src/main/java/com/facebook/presto/server/protocol/Query.java
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/server/protocol/Query.java
Outdated
Show resolved
Hide resolved
presto-tests/src/test/java/com/facebook/presto/tests/TestHistoryBasedRetry.java
Show resolved
Hide resolved
presto-tests/src/test/java/com/facebook/presto/tests/TestHistoryBasedRetry.java
Outdated
Show resolved
Hide resolved
presto-tests/src/test/java/com/facebook/presto/tests/TestHistoryBasedRetry.java
Outdated
Show resolved
Hide resolved
presto-tests/src/test/java/com/facebook/presto/tests/TestHistoryBasedRetry.java
Outdated
Show resolved
Hide resolved
presto-tests/src/test/java/com/facebook/presto/tests/TestHistoryBasedRetry.java
Outdated
Show resolved
Hide resolved
feilong-liu
reviewed
Aug 1, 2024
presto-main/src/main/java/com/facebook/presto/server/protocol/Query.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/server/protocol/Query.java
Outdated
Show resolved
Hide resolved
feilong-liu
reviewed
Aug 1, 2024
presto-tests/src/test/java/com/facebook/presto/tests/TestHistoryBasedRetry.java
Outdated
Show resolved
Hide resolved
presto-tests/src/test/java/com/facebook/presto/tests/TestHistoryBasedRetry.java
Show resolved
Hide resolved
presto-tests/src/test/java/com/facebook/presto/tests/TestHistoryBasedRetry.java
Outdated
Show resolved
Hide resolved
presto-tests/src/test/java/com/facebook/presto/tests/TestHistoryBasedRetry.java
Outdated
Show resolved
Hide resolved
feilong-liu
reviewed
Aug 1, 2024
presto-main/src/main/java/com/facebook/presto/server/protocol/Query.java
Outdated
Show resolved
Hide resolved
feilong-liu
reviewed
Aug 2, 2024
presto-main/src/main/java/com/facebook/presto/server/protocol/Query.java
Outdated
Show resolved
Hide resolved
feilong-liu
reviewed
Aug 2, 2024
presto-main/src/main/java/com/facebook/presto/server/protocol/Query.java
Outdated
Show resolved
Hide resolved
feilong-liu
approved these changes
Aug 2, 2024
34 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Implement retry query support for HBO in Presto, which retries failed queries and tries to retry if the query plan has changed. If the plan has changed it will re-execute, otherwise after the plan comparison we immediately fail it back
Motivation and Context
Many times queries fail because of cases where estimates are inaccurate and various out-of-memory, errors can occur. With this change enabled, users will be able to have an automatic retry which will determine if a retry of the query can prevent their query from failing entirely
Impact
Now when a query fails and a user reruns it, ending up in the query succeeding, with this session property enabled, we can speed up this process to ensure it automatically retries to see if HBO can help the query
Test Plan
Various created unit tests deeply testing different parts of the new and amended code's functionality. Also E2E tested feature within verified cluster
Please message me or Feilong for example of this change working!
Contributor checklist
Release Notes