Handle long running queries in DirectTrinoClient #26371
Merged
Conversation
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
lukasz-stec
reviewed
Aug 8, 2025
Member
lukasz-stec
left a comment
lukasz-stec
left a comment
There was a problem hiding this comment.
Looks good. Please add a test with a small query.client.timeout and a query running longer
core/trino-main/src/main/java/io/trino/client/direct/DirectTrinoClient.java
Outdated
Show resolved
Hide resolved
kokosing
reviewed
Aug 8, 2025
Member
kokosing
left a comment
kokosing
left a comment
There was a problem hiding this comment.
Is it possible to test it it works? For example you can have a query that takes 5 seconds to complete and query.client.timeout set to 1 second, and see that you are able to run such query.
core/trino-main/src/main/java/io/trino/client/direct/DirectTrinoClient.java
Show resolved
Hide resolved
mwd410
force-pushed
the
mdeady/directTrinoClientHeartbeat
branch
3 times, most recently
from
dbba1b5 to
1245cde
Compare
kokosing
approved these changes
Aug 8, 2025
testing/trino-tests/src/test/java/io/trino/client/direct/TestDirectTrinoClient.java
Show resolved
Hide resolved
testing/trino-tests/src/test/java/io/trino/client/direct/TestDirectTrinoClient.java
Outdated
Show resolved
Hide resolved
testing/trino-tests/src/test/java/io/trino/client/direct/TestDirectTrinoClient.java
Outdated
Show resolved
Hide resolved
mwd410
force-pushed
the
mdeady/directTrinoClientHeartbeat
branch
from
1245cde to
3a9fe4a
Compare
lukasz-stec
approved these changes
Aug 8, 2025
testing/trino-tests/src/test/java/io/trino/client/direct/TestDirectTrinoClient.java
Outdated
Show resolved
Hide resolved
Member
|
Ping me when it is green |
mwd410
force-pushed
the
mdeady/directTrinoClientHeartbeat
branch
4 times, most recently
from
8ff4dbd to
b4da9b4
Compare
When a query being executed by DirectTrinoClient takes longer than the query.client.timeout, the query is then canceled. This client is used in some cases when executing queries from within the coordinator
mwd410
force-pushed
the
mdeady/directTrinoClientHeartbeat
branch
from
b4da9b4 to
8d4cdb9
Compare
kokosing
approved these changes
Aug 8, 2025
Member
|
Thanks! |
| catch (TimeoutException e) { | ||
| // continue waiting until the query state changes or the exchange client is blocked. | ||
| // we need to periodically record the heartbeat to prevent the query from being canceled | ||
| dispatchQuery.recordHeartbeat(); |
Contributor
There was a problem hiding this comment.
Nice! I’ve seen abandoned queries before, especially when the cluster was under high pressure — it have been the actual cause back then
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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
When a query being executed by DirectTrinoClient takes longer than the query.client.timeout, the query is then canceled. This client is used in some cases when executing queries from within the coordinator
Additional context and related issues
The
QueryTrackeruses this timeout in itsprivate boolean isAbandoned(T query)method to check it vs the last heartbeat. @lukasz-stec recommended i periodically record a heartbeat while waiting for this future to avoid falling into that case.Release notes
(X) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text: