Skip to content

Conversation

vandop
Copy link
Owner

@vandop vandop commented Sep 24, 2025

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()
    • Reduced 3 blocks of duplicate exception handling to single modular implementation

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>
  • Results: 100% success rate, clean error suppression confirmed

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

Code Pattern Consistency

This change follows the established ARROW-17785 pattern from PreparedStatement.close():

  • Same error detection logic
  • Same suppression conditions
  • Same logging approach
  • Maintains consistency across the codebase

Risk Assessment

Low Risk:

  • Limited scope: Changes only affect teardown/cleanup paths
  • Conservative approach: Only suppresses specific transient conditions
  • Preserves functionality: All genuine errors continue to throw SQLException
  • Follows precedent: Mirrors existing PreparedStatement.close() behavior

Related Issues/Context

  • Follows: ARROW-17785 (PreparedStatement error suppression pattern)
  • Addresses: gRPC channel shutdown race conditions in pooled environments
  • Improves: Compatibility with Apache Commons DBCP and similar pooling frameworks

…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.
@vandop vandop merged commit cef8415 into main Oct 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant