You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "xiangfu0 (via GitHub)" <gi...@apache.org> on 2026/03/25 23:16:42 UTC
[PR] Bump Netty from 4.1.122 to 4.2.11 with compatibility fixes [pinot]
xiangfu0 opened a new pull request, #17980:
URL: https://github.com/apache/pinot/pull/17980
## Summary
- Bump `io.netty:netty-bom` from 4.1.122.Final to 4.2.11.Final
- Fix `QueryServer.shutDown()`: Netty 4.2's `shutdownGracefully(0,0,SECONDS)` no longer force-closes registered channels. Explicitly close all tracked client channels before shutting down event loop groups so remote peers detect the shutdown promptly.
- Fix TLS endpoint verification: Netty 4.2 defaults `SslContextBuilder.endpointIdentificationAlgorithm` to `"HTTPS"` (was `null` in 4.1), enabling hostname verification by default. Explicitly disable it to maintain backwards compatibility for internal cluster communication.
## Test plan
- [x] `QueryRoutingTest` (9 tests) — previously 3 failures (`testServerDown`, `testSkipUnavailableServer`, `testValidResponse`), now all pass
- [x] `QueryServerTest`, `ServerChannelsTest`, `DirectOOMHandlerTest` — all pass
- [ ] `TlsIntegrationTest` — endpoint verification fix should resolve the `setUp` timeout caused by TLS handshake failures between cluster components
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Bump Netty from 4.1.132 to 4.2.11 with compatibility fixes [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 merged PR #17980:
URL: https://github.com/apache/pinot/pull/17980
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Bump Netty from 4.1.132 to 4.2.11 with compatibility fixes [pinot]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #17980:
URL: https://github.com/apache/pinot/pull/17980#discussion_r2992968673
##########
pinot-core/src/main/java/org/apache/pinot/core/transport/QueryServer.java:
##########
@@ -172,6 +172,16 @@ public void shutDown() {
} catch (Exception e) {
throw new RuntimeException(e);
} finally {
+ // Explicitly close all client channels and wait for completion so that remote peers detect
+ // the shutdown promptly (Netty 4.2 no longer force-closes channels during shutdownGracefully
+ // with a zero timeout).
+ for (SocketChannel ch : _allChannels.keySet()) {
+ try {
+ ch.close().sync();
+ } catch (Exception e) {
+ LOGGER.warn("Failed to close client channel: {}", ch, e);
+ }
Review Comment:
This loop blocks on `ch.close().sync()` and catches `Exception`, which includes `InterruptedException`. If the thread is interrupted during shutdown, the interrupt flag is cleared and the shutdown continues without preserving the interruption signal. Handle `InterruptedException` separately (restore `Thread.currentThread().interrupt()` and stop waiting / break), and consider using an uninterruptible wait if interruption should not abort shutdown.
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/tls/TlsUtils.java:
##########
@@ -343,6 +343,9 @@ public static SslContext buildClientContext(TlsConfig tlsConfig) {
SslContextBuilder.forClient().sslProvider(SslProvider.valueOf(tlsConfig.getSslProvider()));
sslFactory.getKeyManagerFactory().ifPresent(sslContextBuilder::keyManager);
sslFactory.getTrustManagerFactory().ifPresent(sslContextBuilder::trustManager);
+ // Netty 4.2 defaults endpointIdentificationAlgorithm to HTTPS (hostname verification enabled).
+ // Apply the value from TlsConfig, which defaults to empty (disabled) for backwards compatibility.
+ sslContextBuilder.endpointIdentificationAlgorithm(tlsConfig.getEndpointIdentificationAlgorithm());
Review Comment:
New behavior is introduced by wiring `TlsConfig.endpointIdentificationAlgorithm` into the Netty `SslContextBuilder`, but there is no unit test coverage asserting the configured value is applied (e.g., that an SSLEngine created from the resulting context has the expected `SSLParameters.getEndpointIdentificationAlgorithm()` for both default/disabled and "HTTPS"). Since `TlsUtils` already has unit tests, please add/extend tests to cover this new configuration path.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Bump Netty from 4.1.122 to 4.2.11 with compatibility fixes [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #17980:
URL: https://github.com/apache/pinot/pull/17980#issuecomment-4130700166
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/17980?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `14.28571%` with `12 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 34.16%. Comparing base ([`56faa36`](https://app.codecov.io/gh/apache/pinot/commit/56faa363989fe4f88eed47f255a335fe586f98aa?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`61fbf2d`](https://app.codecov.io/gh/apache/pinot/commit/61fbf2d8e80b3c72022d179491e0260ad2eb3010?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 9 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/pinot/pull/17980?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...a/org/apache/pinot/core/transport/QueryServer.java](https://app.codecov.io/gh/apache/pinot/pull/17980?src=pr&el=tree&filepath=pinot-core%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcore%2Ftransport%2FQueryServer.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS90cmFuc3BvcnQvUXVlcnlTZXJ2ZXIuamF2YQ==) | 0.00% | [6 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/17980?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...java/org/apache/pinot/common/config/TlsConfig.java](https://app.codecov.io/gh/apache/pinot/pull/17980?src=pr&el=tree&filepath=pinot-common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcommon%2Fconfig%2FTlsConfig.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vY29uZmlnL1Rsc0NvbmZpZy5qYXZh) | 28.57% | [5 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/17980?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...va/org/apache/pinot/common/utils/tls/TlsUtils.java](https://app.codecov.io/gh/apache/pinot/pull/17980?src=pr&el=tree&filepath=pinot-common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcommon%2Futils%2Ftls%2FTlsUtils.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvdGxzL1Rsc1V0aWxzLmphdmE=) | 0.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/17980?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> :exclamation: There is a different number of reports uploaded between BASE (56faa36) and HEAD (61fbf2d). Click for more details.
>
> <details><summary>HEAD has 32 uploads less than BASE</summary>
>
>| Flag | BASE (56faa36) | HEAD (61fbf2d) |
>|------|------|------|
>|java-21|5|1|
>|unittests1|1|0|
>|unittests|3|1|
>|temurin|9|1|
>|unittests2|2|1|
>|java-11|4|0|
>|integration|6|0|
>|integration2|2|0|
>|integration1|2|0|
>|custom-integration1|2|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #17980 +/- ##
=============================================
- Coverage 63.27% 34.16% -29.11%
+ Complexity 1541 789 -752
=============================================
Files 3196 3196
Lines 193744 193740 -4
Branches 29807 29804 -3
=============================================
- Hits 122589 66196 -56393
- Misses 61511 121977 +60466
+ Partials 9644 5567 -4077
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/17980/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [custom-integration1](https://app.codecov.io/gh/apache/pinot/pull/17980/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration](https://app.codecov.io/gh/apache/pinot/pull/17980/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/17980/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/17980/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/17980/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-21](https://app.codecov.io/gh/apache/pinot/pull/17980/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `34.16% <14.28%> (-29.11%)` | :arrow_down: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/17980/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `34.16% <14.28%> (-29.11%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/17980/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `34.16% <14.28%> (-29.11%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/17980/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/17980/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `34.16% <14.28%> (-0.08%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/pinot/pull/17980?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org