chore(deps): Upgrade netty to 4.2.10.Final#27277
Merged
feilong-liu merged 1 commit intoprestodb:masterfrom Mar 6, 2026
Merged
Conversation
Summary: Bump netty to 4.2.10.Final Differential Revision: D95429747
Contributor
Reviewer's GuideUpdates Netty and related networking dependencies to newer versions and adjusts Presto’s Netty/Drift integration and build configuration to be compatible with Netty 4.2, including SSL behavior, allocator wiring, and dependency scoping/exclusions. Class diagram for updated SafeEventLoopGroup integrationclassDiagram
class HttpRemoteTaskFactory {
- Optional~EventLoopGroup~ eventLoopGroup
+ HttpRemoteTaskFactory(...)
}
class SafeEventLoopGroup {
+ SafeEventLoopGroup(int nThreads, ThreadFactory threadFactory, long slowMethodThresholdNanos)
+ newChild(Executor executor, Object args) EventLoop
+ run()
}
class SafeEventLoop {
+ SafeEventLoop(SafeEventLoopGroup parent, Executor executor)
+ run()
}
HttpRemoteTaskFactory --> SafeEventLoopGroup : creates
SafeEventLoopGroup --> SafeEventLoop : newChild
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- Disabling Netty’s SSL endpoint verification globally via a system property in PrestoServer.run() is a broad change; consider scoping this to just the Drift Netty clients (or making it explicitly configurable) so other SSL users can still benefit from hostname verification.
- The change in SafeEventLoop.run() from runTask(task) to task.run() alters how tasks are executed and may bypass any Netty bookkeeping or hooks in runTask; if this is required for 4.2 compatibility, consider adding a short comment explaining why the override is necessary and safe.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Disabling Netty’s SSL endpoint verification globally via a system property in PrestoServer.run() is a broad change; consider scoping this to just the Drift Netty clients (or making it explicitly configurable) so other SSL users can still benefit from hostname verification.
- The change in SafeEventLoop.run() from runTask(task) to task.run() alters how tasks are executed and may bypass any Netty bookkeeping or hooks in runTask; if this is required for 4.2 compatibility, consider adding a short comment explaining why the override is necessary and safe.
## Individual Comments
### Comment 1
<location path="presto-main-base/src/main/java/com/facebook/presto/execution/SafeEventLoopGroup.java" line_range="74-75" />
<code_context>
Runnable task = takeTask();
if (task != null) {
try {
- runTask(task);
+ task.run();
}
catch (Throwable t) {
</code_context>
<issue_to_address>
**issue (bug_risk):** Replacing `runTask(task)` with `task.run()` may bypass Netty’s internal execution/measurement logic.
In `SingleThreadEventExecutor`, `runTask(task)` may perform additional work (e.g., internal accounting, instrumentation, executor state checks). Given this class’s `slowMethodThreshold` and its role as a safe wrapper, calling `task.run()` directly risks bypassing those hooks and breaking existing timing/monitoring behavior. Please consider keeping `runTask(task)` (or the appropriate 4.2 API) and layering your extra safety/metrics on top of it instead.
</issue_to_address>
### Comment 2
<location path="presto-main/src/main/java/com/facebook/presto/server/PrestoServer.java" line_range="123-126" />
<code_context>
verifyJvmRequirements();
verifySystemTimeIsReasonable();
+ // Netty 4.2 enables SSL endpoint verification by default. The Drift Netty transport
+ // does not pass hostnames to the SSL engine, causing SSLHandshakeException. Disable
+ // the default endpoint verification until Drift is updated to support it.
+ if (System.getProperty("io.netty.handler.ssl.defaultEndpointVerificationAlgorithm") == null) {
+ System.setProperty("io.netty.handler.ssl.defaultEndpointVerificationAlgorithm", "NONE");
+ }
</code_context>
<issue_to_address>
**🚨 issue (security):** Disabling Netty’s default endpoint verification globally may be too broad and could weaken TLS guarantees.
This system property applies to all Netty SSL usage in the JVM, so this change weakens TLS verification for every Netty-based client that doesn’t explicitly override the algorithm. Can we instead confine the workaround to Drift’s SSL configuration, or at least guard it with a dedicated config flag so operators must opt in to disabling verification?
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
presto-main-base/src/main/java/com/facebook/presto/execution/SafeEventLoopGroup.java
Show resolved
Hide resolved
amitkdutta
approved these changes
Mar 6, 2026
feilong-liu
approved these changes
Mar 6, 2026
garimauttam
pushed a commit
to garimauttam/presto
that referenced
this pull request
Mar 9, 2026
## Description Upgrade Netty to 4.2.10.Final ## Motivation and Context ## Impact ## Test Plan ## Contributor checklist - [x] Please make sure your submission complies with our [contributing guide](https://github.com/prestodb/presto/blob/master/CONTRIBUTING.md), in particular [code style](https://github.com/prestodb/presto/blob/master/CONTRIBUTING.md#code-style) and [commit standards](https://github.com/prestodb/presto/blob/master/CONTRIBUTING.md#commit-standards). - [x] PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced. - [x] Documented new properties (with its default value), SQL syntax, functions, or other functionality. - [x] If release notes are required, they follow the [release notes guidelines](https://github.com/prestodb/presto/wiki/Release-Notes-Guidelines). - [x] Adequate tests were added if applicable. - [x] CI passed. - [x] If adding new dependencies, verified they have an [OpenSSF Scorecard](https://securityscorecards.dev/#the-checks) score of 5.0 or higher (or obtained explicit TSC approval for lower scores). ## Release Notes ``` == RELEASE NOTES == General Changes * Update netty from version 4.1.130.Final to 4.2.10.Final. ```
This was referenced Mar 31, 2026
15 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
Upgrade Netty to 4.2.10.Final
Motivation and Context
Impact
Test Plan
Contributor checklist
Release Notes