Enable HTTP/2 for internal communication by default #23857
Merged
+245
−408
Conversation
This file contains 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
wendigo
force-pushed
the
serafin/simplify-pages-exchange
branch
3 times, most recently
from
0bc3f9d
to
be4d7c8
Compare
This was used in a single place so it's better to inline it
As it's decoupled from the processing logic so this could race. When timeout is applied on the future, there is a single place that defines a state of the processing.
wendigo
force-pushed
the
serafin/simplify-pages-exchange
branch
from
be4d7c8
to
dded9b6
Compare
losipiuk
reviewed
Oct 22, 2024
core/trino-main/src/main/java/io/trino/server/EofWriterInterceptor.java
Outdated
Show resolved
Hide resolved
losipiuk
reviewed
Oct 22, 2024
core/trino-main/src/main/java/io/trino/server/InternalCommunicationConfig.java
Show resolved
Hide resolved
losipiuk
reviewed
Oct 22, 2024
| @@ -93,8 +85,6 @@ public Response toResponse(Throwable throwable) | |||
| .build(); | |||
| case WebApplicationException webApplicationException -> webApplicationException.getResponse(); | |||
| default -> { | |||
| log.warn(throwable, "Request failed for %s", request.getRequestURI()); | |||
There was a problem hiding this comment.
"Request could be already recycled"
Is that really valid? Was the log message not useful?
There was a problem hiding this comment.
Yes. The ThrowableMapper can be called when the response write has failed - in this case, mapping of the exception to the response doesn't make sense at all as you won't be able to write it anyway
losipiuk
reviewed
Oct 22, 2024
losipiuk
reviewed
Oct 22, 2024
wendigo
changed the title
- Improve error handling - Tune internal HTTP2 settings
It's not available in the JDBC test scope
Request could be already recycled
wendigo
force-pushed
the
serafin/simplify-pages-exchange
branch
from
dded9b6
to
e832e9f
Compare
|
Supersedes #22461 |
losipiuk
approved these changes
Oct 22, 2024
There was a problem hiding this comment.
LGTM - but I want some other people to take a look too.
@electrum, @Praveen2112, @hashhar, @findepi
wendigo
force-pushed
the
serafin/simplify-pages-exchange
branch
2 times, most recently
from
9d7f941
to
2c174ce
Compare
martint
reviewed
Oct 22, 2024
wendigo
force-pushed
the
serafin/simplify-pages-exchange
branch
from
f0b58fa
to
e832e9f
Compare
This was referenced Oct 23, 2024
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
Additional context and related issues
Related to #22271 and #21793
Release notes
( ) 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.
(x) Release notes are required, with the following suggested text: