feat: Add reset() method, gRPC keep-alive, and transport resilience#40
Merged
Conversation
Arrow Flight's default FlightClient.Builder uses
NettyChannelBuilder.forAddress(SocketAddress), which calls
Location.toSocketAddress() -> new InetSocketAddress(host, port).
This eagerly resolves DNS once at construction time and registers a
DirectAddressNameResolverProvider that never re-resolves.
For long-lived clients connecting to load-balanced endpoints (e.g.
AWS ALBs) where backend IPs can change, this causes the gRPC channel
to get stuck on stale IPs indefinitely. If the old IP is recycled to
a different service, the client sees TLS certificate mismatches and
cannot recover without being fully reconstructed.
This change builds the gRPC ManagedChannel directly using
NettyChannelBuilder.forTarget("dns:///host:port") instead of going
through Arrow's FlightClient.Builder. The "dns:///" target scheme
activates gRPC's DnsNameResolver, which periodically re-resolves the
hostname (default 30s cache TTL) and triggers re-resolution on
transient failures via its refresh() method.
The FlightClient is then created via FlightGrpcUtils.createFlightClient()
with the custom channel.
- Add public reset() method to SpiceClient for recovering from unrecoverable transport failures (SSL cert mismatch, persistent UNAVAILABLE errors, stale connections pinned to wrong backend IPs) - Extract buildFlightClient() for reuse during construction and reset - Configure HTTP/2 keep-alive (30s interval, 10s timeout) on the gRPC channel to detect dead/idle connections behind load balancers - Add ensureFlightClient() guard in queryInternal() for safety - Handle null flightClient in close() after reset - Add ResetTest.java with 20 unit tests covering happy path, edge cases, concurrency, construction variants, and integration - Document transport resilience and reset() usage in README
…ns-re-resolution # Conflicts: # src/main/java/ai/spice/SpiceClient.java
- Resolve merge conflicts from trunk merge (buildFlightClient extraction) - Fix TimeUnit import ambiguity: fully qualify java.util.concurrent.TimeUnit at keepAlive call sites to avoid conflict with Arrow's TimeUnit - Add closed guard to reset() — throws IllegalStateException after close() - Make close() idempotent with early return on already-closed client - Update testResetAfterClose to expect IllegalStateException
There was a problem hiding this comment.
Pull request overview
This PR adds transport resilience features to the Java SpiceClient to better handle long-lived connections behind load balancers, including a public reset() API and gRPC keep-alive configuration, along with new tests and README guidance.
Changes:
- Adds
SpiceClient.reset()and refactors Flight client creation intobuildFlightClient()with aensureFlightClient()guard inqueryInternal(). - Configures HTTP/2 keep-alive on the underlying gRPC channel.
- Adds
ResetTestcoverage and documents recommendedreset()usage in the README.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
| src/main/java/ai/spice/SpiceClient.java | Introduces reset(), factors out/reuses Flight client construction, adds keep-alive, and makes close() idempotent/null-safe. |
| src/test/java/ai/spice/ResetTest.java | Adds tests for reset behavior, construction variants, and concurrency/integration scenarios. |
| README.md | Documents long-lived client behavior, keep-alive, DNS behavior, and reset() usage guidance. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Add SpotBugs, OWASP dependency-check, JaCoCo, Enforcer, Checkstyle plugins - Add 'quality' CI job running static analysis and dependency scanning - Add JaCoCo coverage reporting to build_multi_os CI job - Make 'closed' field volatile for thread-safety - Synchronize close() to prevent race with reset()/buildFlightClient() - Add channel cleanup (shutdownNow) on buildFlightClient() failure - Remove unused VectorSchemaRoot import in ResetTest - Fix stale/misleading comments in ResetTest
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
sgrebnov
approved these changes
Mar 25, 2026
Address CodeQL finding: restrict GITHUB_TOKEN to contents:read.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…hroughput Instead of synchronizing the entire queryInternal() method (which would serialize all gRPC RPCs), snapshot flightClient and authCallOptions under a short synchronized block, then execute RPCs without holding the lock. This allows concurrent queries to run in parallel while still being safe against concurrent reset() calls.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Set 10s connect timeout on the static HttpClient used for dataset refresh, preventing threads from blocking indefinitely on unreachable endpoints. - Replace synchronized initADBCIfNeeded() with double-checked locking using a volatile adbcInitialized flag. After warmup, parameterized queries skip the monitor entirely (volatile read only), eliminating contention at high concurrency.
lukekim
force-pushed
the
phillipleblanc/fix-dns-re-resolution
branch
from
March 25, 2026 22:50
dd4afae to
18015f8
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Extract named constants MAX_INBOUND_MESSAGE_SIZE (Integer.MAX_VALUE ≈ 2 GiB) and MAX_INBOUND_METADATA_SIZE (16 MiB) to make the caps explicit and prevent unbounded metadata from consuming heap on large dataset transfers.
lukekim
force-pushed
the
phillipleblanc/fix-dns-re-resolution
branch
from
March 25, 2026 22:59
18015f8 to
137d129
Compare
lukekim
force-pushed
the
phillipleblanc/fix-dns-re-resolution
branch
from
March 25, 2026 22:59
18015f8 to
137d129
Compare
- #5: Eliminate intermediate Object[] and ArrowType[] arrays in parameter binding; build schema fields directly in a single pass and read values from the original params array during vector population. - #6: Close AdbcStatement eagerly after executeQuery() returns the reader. The ArrowReader holds its own Flight stream and no longer needs the statement, freeing server-side resources immediately instead of waiting for slow consumers. - #7: Create auth middleware once per RPC in HeaderAuthMiddlewareFactory instead of re-creating it in each callback (onBeforeSendingHeaders, onHeadersReceived, onCallCompleted). Eliminates 2 redundant allocations per RPC. - #8: Replace message.contains() string scanning in ADBC retry logic with AdbcException.getStatus() switch on AdbcStatusCode enum (IO, UNKNOWN, TIMEOUT, INTERNAL). Avoids string allocation and scanning on the exception hot path.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Add closed guard to ensureFlightClient() to prevent rebuilding transport after client is closed. - Use local temporaries in initADBCIfNeeded() to avoid leaking partially created AdbcDatabase/AdbcConnection on failure. - Wrap FlightStream and ArrowReader in try-with-resources in ResetTest to prevent resource leaks during integration runs. - Add bounded 30s timeout to CountDownLatch.await() in concurrent tests to prevent indefinite hangs on regression. - Scope SpotBugs exclusions: limit example package exclusion to DLS_DEAD_LOCAL_STORE, scope CT_CONSTRUCTOR_THROW to specific classes (SpiceClient, SpiceClientBuilder). - Fix README example to use try-with-resources for FlightStream. - Add OWASP Dependency-Check data caching and NVD API key support in CI to improve reliability and speed.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Close allocator in SpiceClient constructor if buildFlightClient() or initRetryers() throws, preventing off-heap memory leaks on failed client construction. - Replace raw Thread usage in concurrent tests with ExecutorService and proper shutdownNow() cleanup, preventing thread leaks and JVM hangs on test timeout. - Refactor integration tests to probe server availability in setUp() and gate with a boolean flag (matching TpchIntegrationTest pattern) instead of brittle exception message substring matching. - Clarify README example: add comment explaining isTransportFailure() is application-defined and suggest which exception types to check.
…emove stale .commitmsg - Add CNT_ROUGH_CONSTANT_VALUE SpotBugs exclusion for example code (3.14/3.14159265359 are intentional demo values for float params) - Change ResetTest integration tests to use taxi_trips table (available in CI quickstart dataset) instead of tpch.customer (not available) - Remove .commitmsg that was accidentally re-added
phillipleblanc
force-pushed
the
phillipleblanc/fix-dns-re-resolution
branch
from
March 26, 2026 01:17
92afb9b to
6f55f6c
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Make server availability probe static/one-time in ResetTest (avoids redundant client+query per test method) - Use CopyOnWriteArrayList in concurrent reset+query test for thread safety - Add closed guard in queryWithParams() for consistent IllegalStateException instead of confusing NPE after close()
phillipleblanc
force-pushed
the
phillipleblanc/fix-dns-re-resolution
branch
from
March 26, 2026 03:10
f6524e4 to
5a464d8
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Changes
reset()method toSpiceClientfor recovering from unrecoverable transport failures (SSL cert mismatch, persistent UNAVAILABLE errors, stale connections pinned to wrong backend IPs)buildFlightClient()for reuse during construction and afterreset()ensureFlightClient()guard inqueryInternal()for safetyflightClientinclose()after resetResetTest.javawith 20 unit tests covering happy path, edge cases, concurrency, construction variants, and integrationreset()usage in README