GH-863: [JDBC] Suppress benign exceptions from gRPC layer on ArrowFlightSqlClientHandler#close #910
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
…hen catalog is set This change addresses race conditions during gRPC channel shutdown that occur when using connection pooling with catalog parameters. The CloseSession RPC can fail with UNAVAILABLE or 'Connection closed after GOAWAY' errors during normal connection cleanup. Key improvements: - Refactored duplicate exception handling code into reusable helper methods - Added comprehensive error suppression for both AutoCloseable cleanup and CloseSession - Follows the established ARROW-17785 pattern from PreparedStatement.close() - Improved logging with context-aware debug/info messages - Fixed typo in existing error suppression logging The refactoring eliminates code duplication while maintaining identical functionality and improving maintainability.
…on error suppression Add unit and integration tests for the error suppression functionality in ArrowFlightSqlClientHandler: - ArrowFlightSqlClientHandlerTest: 18 unit tests covering error detection, logging, and exception handling logic using Mockito and reflection - ArrowFlightSqlClientHandlerIntegrationTest: 4 integration tests with real FlightServer to validate error suppression in realistic scenarios Tests verify that benign gRPC shutdown errors (UNAVAILABLE and INTERNAL with GOAWAY) are properly suppressed while genuine failures are correctly propagated as exceptions.
…reliability This commit addresses bugs introduced in the error suppression implementation: 1. Fixed NullPointerException in isBenignCloseException() when FlightRuntimeException.getMessage() returns null. Added null check before calling contains() on the message string. 2. Fixed unit test setup to avoid attempting real server connections during test initialization. Tests now use reflection to test private methods without requiring actual network connections. 3. Fixed Mockito unnecessary stubbing warnings by making all mock objects lenient, allowing tests to create comprehensive mocks without triggering warnings when not all stubbings are used. 4. Simplified integration tests to focus on testable scenarios. Removed tests that required mocking gRPC service methods (closeSession) which are not routed through FlightProducer, making them difficult to test in isolation. Test Results: - 21 tests total (15 unit + 1 integration + 5 builder tests) - All tests passing with 0 failures and 0 errors - Comprehensive coverage of error suppression logic via reflection-based unit tests
This comment has been minimized.
This comment has been minimized.
ennuite
requested review from
jbonofre,
laurentgo,
lidavidm and
wgtmac
as code owners
ennuite
changed the title
Contributor
Author
|
This PR is based on https://github.com/apache/arrow-java/pull/864/files, @vandop asked me to take it over from him. I simplified the tests. I don't think this merits a full integration test with shutting down a server because all the logic is inside a single method. About the logging level: for the original issue at https://github.com/apache/arrow/pull/14210/files the log level was set to warn, and in Vando's PR @lidavidm asked for the log level to be debug. It's not clear to me the reason for the discrepancy, and I put it as info. |
vandop
approved these changes
Nov 25, 2025
vandop
left a comment
vandop
left a comment
There was a problem hiding this comment.
Thanks for taking over.
Note internally in Dremio we already tested this with jmeter, which wouldn't run without this change.
Member
|
The format is not fully correct. I will fix that. |
lidavidm
reviewed
Nov 26, 2025
| private void logSuppressedCloseException( | ||
| FlightRuntimeException fre, String operationDescription) { | ||
| // ARROW-17785 and GH-863: suppress exceptions caused by flaky gRPC layer during shutdown | ||
| LOGGER.info("Suppressed error {}", operationDescription, fre); |
Member
There was a problem hiding this comment.
I would still prefer debug level
Member
There was a problem hiding this comment.
Agreed. Debug makes more sense to me here.
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.
What's Changed
When using the Flight SQL JDBC driver with connection pooling and a catalog parameter, ArrowFlightSqlClientHandler.close() performs a CloseSession RPC that can fail during gRPC channel shutdown.
These transient failures (UNAVAILABLE or INTERNAL with "Connection closed after GOAWAY") cause noisy errors in pooling frameworks like Apache Commons DBCP.
With this PR these exceptions will instead be suppressed and logged, following the procedure that was used for ARROW-17785
Are these changes tested?
Yes
Closes #863