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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
