You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "vandop (via GitHub)" <gi...@apache.org> on 2025/10/01 15:22:47 UTC
[PR] GH-863: [Flight SQL][JDBC] Suppress benign CloseSession errors when catalog is set [arrow-java]
vandop opened a new pull request, #864:
URL: https://github.com/apache/arrow-java/pull/864
fixes #863
## Rationale for this change
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") bubble up as `SQLException` and cause noisy errors in pooling frameworks like Apache Commons DBCP.
## What changes are included in this PR?
### Core Changes
- **Enhanced error suppression** in `ArrowFlightSqlClientHandler.close()` for both:
- `CloseSession` RPC calls
- `AutoCloseable` resource cleanup
- **Code refactoring** to eliminate duplication:
- Created reusable helper methods: `isBenignCloseException()`, `logSuppressedCloseException()`, `handleBenignCloseException()`
### Error Suppression Logic
Suppress only these specific transient shutdown conditions:
- `FlightStatusCode.UNAVAILABLE`
- `FlightStatusCode.INTERNAL` with message containing "Connection closed after GOAWAY"
### Logging Improvements
- **Context-aware logging**: DEBUG mode shows full stack traces, INFO mode shows clean messages
- **Fixed typo**: "Supressed" → "Suppressed" in existing PreparedStatement error handling
- **Consistent format**: All suppression follows same logging pattern
## Are there any user-facing changes?
**Yes - Improved user experience:**
- **Eliminates noisy shutdown errors** when using catalog parameters with connection pooling
- **Maintains error visibility** for genuine failures (non-transient errors still throw `SQLException`)
- **Better compatibility** with pooling frameworks (JMeter, DBCP, etc.)
## How was this patch tested?
### Integration Testing
- **Environment**: JDK 21, JMeter 5.6.3 with Apache Commons DBCP
- **Target**: Dremio Cloud (`data.dremio.cloud:443`)
- **Test URL**: `jdbc:arrow-flight-sql://data.dremio.cloud:443/?token=<PAT>&catalog=<PROJECT_ID>`
- **JMeter Dremio test suite**: Dremio performance tests were patched with a custom build with this change and are back to a working state (previously were using the legacy Dremio JDBC driver)
### Validation Steps
1. **Before patch**: Intermittent `UNAVAILABLE`/`INTERNAL("Connection closed after GOAWAY")` errors during connection close
2. **After patch**: Benign errors logged as INFO, genuine failures still surface as `SQLException`
3. **Regression testing**: All existing functionality preserved
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-863: [Flight SQL][JDBC] Suppress benign CloseSession errors when catalog is set [arrow-java]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #864:
URL: https://github.com/apache/arrow-java/pull/864#issuecomment-3447992365
Can the PR description also be condensed? It's excessively verbose and essentially lists out every change in the PR itself in prose.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-863: [Flight SQL][JDBC] Suppress benign CloseSession errors when catalog is set [arrow-java]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #864:
URL: https://github.com/apache/arrow-java/pull/864#issuecomment-3406938927
<!-- label_helper_comment -->
Thank you for opening a pull request!
Please label the PR with one or more of:
- bug-fix
- chore
- dependencies
- documentation
- enhancement
Also, add the 'breaking-change' label if appropriate.
See [CONTRIBUTING.md](https://github.com/apache/arrow-java/blob/main/CONTRIBUTING.md) for 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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-863: [Flight SQL][JDBC] Suppress benign CloseSession errors when catalog is set [arrow-java]
Posted by "vandop (via GitHub)" <gi...@apache.org>.
vandop commented on PR #864:
URL: https://github.com/apache/arrow-java/pull/864#issuecomment-3406960921
> Is it possible to unit test this?
I've added some tests now, trying to follow the existing patterns.
The robot is also checking for a label, which I assume someone with write permission has to add it, right?
This would be a bug-fix from my perspective.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-863: [Flight SQL][JDBC] Suppress benign CloseSession errors when catalog is set [arrow-java]
Posted by "vandop (via GitHub)" <gi...@apache.org>.
vandop commented on PR #864:
URL: https://github.com/apache/arrow-java/pull/864#issuecomment-3574743924
Closing this one in favor of @ennuite one's who kindly took over https://github.com/apache/arrow-java/pull/910
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-863: [Flight SQL][JDBC] Suppress benign CloseSession errors when catalog is set [arrow-java]
Posted by "vandop (via GitHub)" <gi...@apache.org>.
vandop closed pull request #864: GH-863: [Flight SQL][JDBC] Suppress benign CloseSession errors when catalog is set
URL: https://github.com/apache/arrow-java/pull/864
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-863: [Flight SQL][JDBC] Suppress benign CloseSession errors when catalog is set [arrow-java]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #864:
URL: https://github.com/apache/arrow-java/pull/864#discussion_r2463184450
##########
flight/flight-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/client/ArrowFlightSqlClientHandlerTest.java:
##########
Review Comment:
I know I asked for tests, but I think this + the other file are excessive compared to the change itself. I would expect something like shutting down the server, followed by shutting down the client, does not raise an error (I assume the original problem was that this would raise an unnecessary error?)
##########
flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/client/ArrowFlightSqlClientHandler.java:
##########
@@ -262,15 +262,83 @@ public FlightInfo getInfo(final String query) {
@Override
public void close() throws SQLException {
if (catalog.isPresent()) {
- sqlClient.closeSession(new CloseSessionRequest(), getOptions());
+ try {
+ sqlClient.closeSession(new CloseSessionRequest(), getOptions());
+ } catch (FlightRuntimeException fre) {
+ handleBenignCloseException(fre, "Failed to close Flight SQL session.", "closing Flight SQL session");
+ }
}
try {
AutoCloseables.close(sqlClient);
+ } catch (FlightRuntimeException fre) {
+ handleBenignCloseException(fre, "Failed to clean up client resources.", "closing Flight SQL client");
} catch (final Exception e) {
throw new SQLException("Failed to clean up client resources.", e);
}
}
+ /**
+ * Handles FlightRuntimeException during close operations, suppressing benign gRPC shutdown errors
+ * while re-throwing genuine failures.
+ *
+ * @param fre the FlightRuntimeException to handle
+ * @param sqlErrorMessage the SQLException message to use for genuine failures
+ * @param operationDescription description of the operation for logging
+ * @throws SQLException if the exception represents a genuine failure
+ */
+ private void handleBenignCloseException(FlightRuntimeException fre, String sqlErrorMessage, String operationDescription) throws SQLException {
+ if (isBenignCloseException(fre)) {
+ logSuppressedCloseException(fre, operationDescription);
+ } else {
+ throw new SQLException(sqlErrorMessage, fre);
+ }
+ }
+
+ /**
+ * Handles FlightRuntimeException during close operations, suppressing benign gRPC shutdown errors
+ * while re-throwing genuine failures as FlightRuntimeException.
+ *
+ * @param fre the FlightRuntimeException to handle
+ * @param operationDescription description of the operation for logging
+ * @throws FlightRuntimeException if the exception represents a genuine failure
+ */
+ private void handleBenignCloseException(FlightRuntimeException fre, String operationDescription) throws FlightRuntimeException {
+ if (isBenignCloseException(fre)) {
+ logSuppressedCloseException(fre, operationDescription);
+ } else {
+ throw fre;
+ }
+ }
+
+ /**
+ * Determines if a FlightRuntimeException represents a benign close operation error
+ * that should be suppressed.
+ *
+ * @param fre the FlightRuntimeException to check
+ * @return true if the exception should be suppressed, false otherwise
+ */
+ private boolean isBenignCloseException(FlightRuntimeException fre) {
+ return fre.status().code().equals(FlightStatusCode.UNAVAILABLE)
+ || (fre.status().code().equals(FlightStatusCode.INTERNAL)
+ && fre.getMessage() != null
+ && fre.getMessage().contains("Connection closed after GOAWAY"));
+ }
+
+ /**
+ * Logs a suppressed close exception with appropriate level based on debug settings.
+ *
+ * @param fre the FlightRuntimeException being suppressed
+ * @param operationDescription description of the operation for logging
+ */
+ private void logSuppressedCloseException(FlightRuntimeException fre, String operationDescription) {
+ // ARROW-17785: suppress exceptions caused by flaky gRPC layer during shutdown
+ if (LOGGER.isDebugEnabled()) {
+ LOGGER.debug("Suppressed error {}", operationDescription, fre);
+ } else {
+ LOGGER.info("Suppressed benign error {}: {}", operationDescription, fre.getMessage());
+ }
Review Comment:
Why the switch? Just always put it at debug level.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org