Fix Netty deprecation warnings in transport-netty4 module #20233
Conversation
Signed-off-by: Sergei Ustimenko <[email protected]>
WalkthroughThese changes standardize Netty channel options, migrate event loop groups to MultiThreadIoEventLoopGroup, adjust pooled buffer allocator construction, and explicitly set the HTTP decompressor buffer size. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
modules/transport-netty4/src/main/java/org/opensearch/transport/SharedGroupFactory.java (1)
49-49: Unused import:NioEventLoopGroupappears to be dead code.After migrating to
MultiThreadIoEventLoopGroup, theNioEventLoopGroupimport is no longer referenced in this file and should be removed.-import io.netty.channel.nio.NioEventLoopGroup;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HttpServerTransport.java(3 hunks)modules/transport-netty4/src/main/java/org/opensearch/transport/NettyAllocator.java(0 hunks)modules/transport-netty4/src/main/java/org/opensearch/transport/SharedGroupFactory.java(3 hunks)modules/transport-netty4/src/main/java/org/opensearch/transport/netty4/Netty4Transport.java(2 hunks)
💤 Files with no reviewable changes (1)
- modules/transport-netty4/src/main/java/org/opensearch/transport/NettyAllocator.java
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: reta
Repo: opensearch-project/OpenSearch PR: 20017
File: plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java:256-256
Timestamp: 2025-12-12T18:40:01.303Z
Learning: In the OpenSearch ReactorNetty4 secure HTTP transport tests (plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java), URI limit validation has been moved from the protocol layer to the transport layer, making it protocol-agnostic. The random protocol selection in ReactorHttpClient.https(settings) is intentional to ensure all tests validate correct behavior across HTTP/1.1, HTTP/2, and HTTP/3.
Learnt from: reta
Repo: opensearch-project/OpenSearch PR: 20017
File: modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4Http3ServerTransport.java:101-123
Timestamp: 2025-12-13T20:16:08.819Z
Learning: In OpenSearch, only one HTTP transport implementation can be active and loaded at a time, so duplicate setting definitions (such as h3.max_stream_local_length, h3.max_stream_remote_length, and h3.max_streams) across different transport implementations like Netty4Http3ServerTransport and ReactorNetty4HttpServerTransport will not cause setting registration conflicts.
📚 Learning: 2025-12-13T20:16:08.819Z
Learnt from: reta
Repo: opensearch-project/OpenSearch PR: 20017
File: modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4Http3ServerTransport.java:101-123
Timestamp: 2025-12-13T20:16:08.819Z
Learning: In OpenSearch, only one HTTP transport implementation can be active and loaded at a time, so duplicate setting definitions (such as h3.max_stream_local_length, h3.max_stream_remote_length, and h3.max_streams) across different transport implementations like Netty4Http3ServerTransport and ReactorNetty4HttpServerTransport will not cause setting registration conflicts.
Applied to files:
modules/transport-netty4/src/main/java/org/opensearch/transport/netty4/Netty4Transport.javamodules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HttpServerTransport.javamodules/transport-netty4/src/main/java/org/opensearch/transport/SharedGroupFactory.java
📚 Learning: 2025-12-12T18:40:01.303Z
Learnt from: reta
Repo: opensearch-project/OpenSearch PR: 20017
File: plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java:256-256
Timestamp: 2025-12-12T18:40:01.303Z
Learning: In the OpenSearch ReactorNetty4 secure HTTP transport tests (plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java), URI limit validation has been moved from the protocol layer to the transport layer, making it protocol-agnostic. The random protocol selection in ReactorHttpClient.https(settings) is intentional to ensure all tests validate correct behavior across HTTP/1.1, HTTP/2, and HTTP/3.
Applied to files:
modules/transport-netty4/src/main/java/org/opensearch/transport/netty4/Netty4Transport.javamodules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HttpServerTransport.javamodules/transport-netty4/src/main/java/org/opensearch/transport/SharedGroupFactory.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: gradle-check
- GitHub Check: assemble (25, windows-latest)
- GitHub Check: assemble (21, ubuntu-latest)
- GitHub Check: precommit (25, ubuntu-latest)
- GitHub Check: precommit (21, ubuntu-latest)
- GitHub Check: precommit (21, windows-2025, true)
- GitHub Check: assemble (21, ubuntu-24.04-arm)
- GitHub Check: assemble (25, ubuntu-24.04-arm)
- GitHub Check: precommit (25, macos-15-intel)
- GitHub Check: precommit (25, windows-latest)
- GitHub Check: precommit (21, macos-15-intel)
- GitHub Check: precommit (25, ubuntu-24.04-arm)
- GitHub Check: precommit (21, windows-latest)
- GitHub Check: assemble (21, windows-latest)
- GitHub Check: precommit (21, macos-15)
- GitHub Check: precommit (25, macos-15)
- GitHub Check: precommit (21, ubuntu-24.04-arm)
- GitHub Check: assemble (25, ubuntu-latest)
- GitHub Check: detect-breaking-change
- GitHub Check: Analyze (java)
🔇 Additional comments (6)
modules/transport-netty4/src/main/java/org/opensearch/transport/netty4/Netty4Transport.java (2)
221-221: LGTM! Deprecation fix for client bootstrap.The rename from
RCVBUF_ALLOCATORtoRECVBUF_ALLOCATORaddresses the Netty deprecation warning. This is a pure constant rename with no functional change.
291-292: LGTM! Deprecation fix for server bootstrap.Both the server channel option and child option are correctly updated to use
RECVBUF_ALLOCATOR, maintaining consistency with the client bootstrap change.modules/transport-netty4/src/main/java/org/opensearch/transport/SharedGroupFactory.java (1)
94-111: LGTM! Migration toMultiThreadIoEventLoopGroupwithNioIoHandler.newFactory()is correct.The migration from
NioEventLoopGrouptoMultiThreadIoEventLoopGroupfollows Netty 4.2's guidance for decoupling transport from event loop groups. UsingNioIoHandler.newFactory()maintains NIO-based I/O handling, preserving behavioral compatibility.modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HttpServerTransport.java (3)
583-585: Behavior change: verify passing 0 matches or improves upon default constructor behavior.The method now explicitly passes
UNLIMITED_DECOMPRESSOR_BUFFER(0) toHttpContentDecompressorinstead of using the default constructor. Since this is an extension point for NetworkPlugin implementations, ensure this change is intentional and that the behavior is equivalent or improved.
319-320: Correct use of non-deprecated Netty 4.2 channel option.The change from
RCVBUF_ALLOCATORtoRECVBUF_ALLOCATORis correct. In Netty 4.2,ChannelOption.RCVBUF_ALLOCATORis deprecated—it is a deprecated alias forRECVBUF_ALLOCATOR. Replacing it withRECVBUF_ALLOCATORproperly resolves the deprecation warning with no behavioral change.
142-146: The constantUNLIMITED_DECOMPRESSOR_BUFFER = 0is correctly implemented. Per Netty's documentation, passing0toHttpContentDecompressor'smaxAllocationparameter means unlimited buffer size, which aligns with the constant's name and intended usage increateDecompressor()at line 584.
Signed-off-by: Sergei Ustimenko <[email protected]>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #20233 +/- ##
=========================================
Coverage 73.20% 73.21%
+ Complexity 71766 71735 -31
=========================================
Files 5795 5795
Lines 328302 328304 +2
Branches 47283 47283
=========================================
+ Hits 240345 240352 +7
- Misses 68628 68668 +40
+ Partials 19329 19284 -45 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1 participant
Description
This PR fixes some of the deprecation warnings in the
transport-netty4module:ChannelOption#RECVBUF_ALLOCATORinstead ofChannelOption#RCVBUF_ALLOCATORPooledByteBufAllocator#defaultTinyCacheSize(as per the comment:@deprecated Tiny caches have been merged into small caches.) + use appropriatePooledByteBufAllocatorconstructor.MultiThreadIoEventLoopGroupinstead ofNioEventLoopGroup. See more here: Netty 4.2 Migration Guide: IoHandlerFactories for EventLoopGroupsHttpContentDecompressor(int maxAllocation)instead of default constructor that internally delegates to theHttpContentDecompressor(boolean strict = false, int maxAllocation = 0). The constructor being used after migration delegates to the same 2-args constructor with thestrict=falseand the givenmaxAllocation = 0which means the maximum size is not limited. This change should be compatible with the previous usage.Related Issues
No issue was created but I can totally do that.
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.
Summary by CodeRabbit
Bug Fixes
Performance Improvements
✏️ Tip: You can customize this high-level summary in your review settings.