3.x: fail fast on keyspace setup validation failures#942
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis change updates keyspace setup failure handling across Sequence Diagram(s)sequenceDiagram
participant HostConnectionPool
participant Connection
participant RequestHandler
HostConnectionPool->>Connection: setKeyspaceAsync("USE keyspace")
Connection->>Connection: write USE query
alt ConnectionException or BusyConnectionException
Connection->>Connection: reset targetKeyspace and fail ksFuture
HostConnectionPool->>HostConnectionPool: release borrowed connection
HostConnectionPool-->>RequestHandler: failed future
else USE returns ERROR
Connection->>Connection: asException(endPoint)
alt QueryValidationException
Connection-->>HostConnectionPool: fail ksFuture without defuncting
HostConnectionPool-->>RequestHandler: failed future
RequestHandler->>RequestHandler: stop speculative retry
else other error
Connection->>Connection: defunct(exception)
Connection-->>HostConnectionPool: fail ksFuture
HostConnectionPool-->>RequestHandler: failed future
RequestHandler->>RequestHandler: query next host
end
end
Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR fixes a retry-bypass bug where synchronous failures during session keyspace setup (USE "<keyspace>") could escape before borrowConnection() returns a future, preventing RequestHandler from using its normal retry/onFailure path. The change makes those failures consistently surface as failed futures and ensures pool accounting is properly restored when keyspace setup fails after a connection has been reserved.
Changes:
- Convert synchronous
ConnectionException/BusyConnectionExceptionfrom the internal keyspaceUSEwrite into a failedsetKeyspaceAsync()future and clear the in-flight keyspace attempt state. - Ensure
HostConnectionPool.borrowConnection()releases the reserved connection (restoringinFlight/totalInFlight) if keyspace setup fails. - Add/extend regression tests covering borrow-time failure behavior and pool counter cleanup.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| driver-core/src/main/java/com/datastax/driver/core/HostConnectionPool.java | Releases reserved connections when keyspace setup fails during borrow; adds defensive handling in dequeue path. |
| driver-core/src/main/java/com/datastax/driver/core/Connection.java | Converts synchronous keyspace setup write failures into failed futures and resets stale keyspace attempts. |
| driver-core/src/test/java/com/datastax/driver/core/HostConnectionPoolTest.java | Adds regression coverage for borrow-time keyspace failures and verifies pool counters are restored. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
dkropachev
force-pushed
the
issue-940-keyspace-setup-retry
branch
from
73957f5 to
087fe8c
Compare
dkropachev
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
driver-core/src/main/java/com/datastax/driver/core/HostConnectionPool.java (1)
717-724: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLikely unreachable given the
Connection.setKeyspaceAsyncfix.Since
Connection.setKeyspaceAsyncnow catchesConnectionException/BusyConnectionExceptionfromwrite()internally and always returns a (possibly already-failed) future rather than throwing, thistry/catcharound the call itself should no longer be exercised in production — the pre-existingsetKeyspaceFuture.isDone()branch below (which unwrapsExecutionExceptionfrom the already-failed future) is what actually handles this scenario now, as evidenced by the new assertions in theshould_fail_in_dequeue_when_setting_keyspace_and_another_set_keyspace_attempt_is_in_flighttest not going through this catch block.This is harmless defense-in-depth (e.g. protects against a mock/spy throwing directly, or a future regression reintroducing a synchronous throw), so not blocking, but worth confirming intent.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@driver-core/src/main/java/com/datastax/driver/core/HostConnectionPool.java` around lines 717 - 724, The try/catch around Connection.setKeyspaceAsync in HostConnectionPool.dequeueBorrow is likely dead code now that setKeyspaceAsync handles ConnectionException and BusyConnectionException internally and returns a failed future instead of throwing. Confirm the intended behavior and, if synchronous throws are no longer expected, remove the catch block and rely on the existing setKeyspaceFuture.isDone() / ExecutionException handling path; otherwise keep it only if you explicitly want defense-in-depth for direct throws from Connection.setKeyspaceAsync.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@driver-core/src/main/java/com/datastax/driver/core/HostConnectionPool.java`:
- Around line 717-724: The try/catch around Connection.setKeyspaceAsync in
HostConnectionPool.dequeueBorrow is likely dead code now that setKeyspaceAsync
handles ConnectionException and BusyConnectionException internally and returns a
failed future instead of throwing. Confirm the intended behavior and, if
synchronous throws are no longer expected, remove the catch block and rely on
the existing setKeyspaceFuture.isDone() / ExecutionException handling path;
otherwise keep it only if you explicitly want defense-in-depth for direct throws
from Connection.setKeyspaceAsync.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 76c38d55-cedd-4668-91e4-84e59c57f8a4
📒 Files selected for processing (3)
driver-core/src/main/java/com/datastax/driver/core/Connection.javadriver-core/src/main/java/com/datastax/driver/core/HostConnectionPool.javadriver-core/src/test/java/com/datastax/driver/core/HostConnectionPoolTest.java
dkropachev
force-pushed
the
issue-940-keyspace-setup-retry
branch
from
087fe8c to
fa922ae
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
dkropachev
force-pushed
the
issue-940-keyspace-setup-retry
branch
from
fa922ae to
5e64ad3
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
dkropachev
force-pushed
the
issue-940-keyspace-setup-retry
branch
from
5e64ad3 to
44883ca
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
dkropachev
force-pushed
the
issue-940-keyspace-setup-retry
branch
from
44883ca to
b725410
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
dkropachev
force-pushed
the
issue-940-keyspace-setup-retry
branch
from
b725410 to
4153ad7
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
driver-core/src/main/java/com/datastax/driver/core/HostConnectionPool.java (1)
616-617: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRestore accounting if
setKeyspaceAsyncstill throws synchronously.
Connection.setKeyspaceAsyncstill rethrowsRuntimeException; if that happens here, Line 592 and Line 596 have already reservedinFlightandtotalInFlight, but the failure callback is never registered. Return a failed future after releasing the reservation.🐛 Proposed fix
final Connection borrowedConnection = leastBusy; - ListenableFuture<Connection> setKeyspaceFuture = - borrowedConnection.setKeyspaceAsync(manager.poolsState.keyspace); + ListenableFuture<Connection> setKeyspaceFuture; + try { + setKeyspaceFuture = borrowedConnection.setKeyspaceAsync(manager.poolsState.keyspace); + } catch (BusyConnectionException e) { + borrowedConnection.release(true); + return Futures.immediateFailedFuture(e); + } catch (RuntimeException e) { + borrowedConnection.release(false); + return Futures.immediateFailedFuture(e); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@driver-core/src/main/java/com/datastax/driver/core/HostConnectionPool.java` around lines 616 - 617, In HostConnectionPool’s connection-borrowing flow, `borrowedConnection.setKeyspaceAsync(...)` can still throw synchronously after `inFlight` and `totalInFlight` have been reserved, leaving the counters unbalanced. Wrap the `setKeyspaceAsync` call in the existing borrow path, and if it throws, immediately release the reservation by decrementing the same accounting state before returning a failed future instead of proceeding to register callbacks. Use the `setKeyspaceFuture` creation site and the surrounding `inFlight`/`totalInFlight` bookkeeping to place the fix.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@driver-core/src/main/java/com/datastax/driver/core/HostConnectionPool.java`:
- Around line 616-617: In HostConnectionPool’s connection-borrowing flow,
`borrowedConnection.setKeyspaceAsync(...)` can still throw synchronously after
`inFlight` and `totalInFlight` have been reserved, leaving the counters
unbalanced. Wrap the `setKeyspaceAsync` call in the existing borrow path, and if
it throws, immediately release the reservation by decrementing the same
accounting state before returning a failed future instead of proceeding to
register callbacks. Use the `setKeyspaceFuture` creation site and the
surrounding `inFlight`/`totalInFlight` bookkeeping to place the fix.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c38f3658-8e5e-49a9-8d9a-64a91232052f
📒 Files selected for processing (4)
driver-core/src/main/java/com/datastax/driver/core/Connection.javadriver-core/src/main/java/com/datastax/driver/core/HostConnectionPool.javadriver-core/src/main/java/com/datastax/driver/core/RequestHandler.javadriver-core/src/test/java/com/datastax/driver/core/HostConnectionPoolTest.java
🚧 Files skipped from review as they are similar to previous changes (2)
- driver-core/src/main/java/com/datastax/driver/core/RequestHandler.java
- driver-core/src/main/java/com/datastax/driver/core/Connection.java
dkropachev
force-pushed
the
issue-940-keyspace-setup-retry
branch
from
4153ad7 to
3f8ffaa
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
driver-core/src/test/java/com/datastax/driver/core/HostConnectionPoolTest.java (1)
606-790: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting the repeated borrow-failure setup into a helper.
The four new tests in this range (
should_fail_fast_when_keyspace_setup_fails_with_validation_error,should_not_call_retry_policy_when_keyspace_setup_fails_with_server_error_before_query_write,should_not_call_retry_policy_when_keyspace_setup_fails_before_query_write,should_try_next_host_when_single_connection_pool_fails_keyspace_setup) repeat the same spy/replace-connection/poolsState.setKeyspace/doReturn(immediateFailedFuture(...))boilerplate. A small helper (e.g.,spyConnectionWithFailedKeyspaceSetup(pool, keyspace, failure)) would reduce duplication.Logic and assertions themselves are correct and consistent with the described
setKeyspaceAsynccontract.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@driver-core/src/test/java/com/datastax/driver/core/HostConnectionPoolTest.java` around lines 606 - 790, The new keyspace setup tests duplicate the same spy/replace-connection/poolsState keyspace/doReturn failed future boilerplate, so extract that repeated setup into a small helper to keep the tests concise and consistent. Add a helper near the affected `HostConnectionPoolTest` cases that takes the `HostConnectionPool`, keyspace name, and failure, then spies the connection, swaps it into `pool.connections[0]`, calls `session.poolsState.setKeyspace(...)`, and stubs `setKeyspaceAsync(...)` to return the failed future. Update `should_fail_fast_when_keyspace_setup_fails_with_validation_error`, `should_not_call_retry_policy_when_keyspace_setup_fails_with_server_error_before_query_write`, `should_not_call_retry_policy_when_keyspace_setup_fails_before_query_write`, and `should_try_next_host_when_single_connection_pool_fails_keyspace_setup` to use the helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@driver-core/src/test/java/com/datastax/driver/core/HostConnectionPoolTest.java`:
- Around line 606-790: The new keyspace setup tests duplicate the same
spy/replace-connection/poolsState keyspace/doReturn failed future boilerplate,
so extract that repeated setup into a small helper to keep the tests concise and
consistent. Add a helper near the affected `HostConnectionPoolTest` cases that
takes the `HostConnectionPool`, keyspace name, and failure, then spies the
connection, swaps it into `pool.connections[0]`, calls
`session.poolsState.setKeyspace(...)`, and stubs `setKeyspaceAsync(...)` to
return the failed future. Update
`should_fail_fast_when_keyspace_setup_fails_with_validation_error`,
`should_not_call_retry_policy_when_keyspace_setup_fails_with_server_error_before_query_write`,
`should_not_call_retry_policy_when_keyspace_setup_fails_before_query_write`, and
`should_try_next_host_when_single_connection_pool_fails_keyspace_setup` to use
the helper.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2ad98183-e87b-4181-bb09-ab0f4af4a5ca
📒 Files selected for processing (4)
driver-core/src/main/java/com/datastax/driver/core/Connection.javadriver-core/src/main/java/com/datastax/driver/core/HostConnectionPool.javadriver-core/src/main/java/com/datastax/driver/core/RequestHandler.javadriver-core/src/test/java/com/datastax/driver/core/HostConnectionPoolTest.java
🚧 Files skipped from review as they are similar to previous changes (3)
- driver-core/src/main/java/com/datastax/driver/core/RequestHandler.java
- driver-core/src/main/java/com/datastax/driver/core/HostConnectionPool.java
- driver-core/src/main/java/com/datastax/driver/core/Connection.java
Fixes #940. Part of #939. Jira: https://scylladb.atlassian.net/browse/DRIVER-751 Keyspace setup during connection borrow runs an internal USE before the user query is written. Treat validation failures from that USE as permanent request errors instead of connection or host failures: preserve QueryValidationException, avoid defuncting the connection, and stop the request without trying the next host or invoking retry policy. Keep driver-side and transient server-side keyspace setup failures on the existing pre-query next-host path. Report synchronous setKeyspaceAsync write failures through failed futures, clear failed attempts, and restore pool accounting for borrow and dequeue failures. Add regression coverage for 0x2200 INVALID responses, synchronous setKeyspace validation handling, client/server-side keyspace setup timeouts, pool accounting cleanup, retry-policy accounting, and next-host behavior.
dkropachev
force-pushed
the
issue-940-keyspace-setup-retry
branch
from
a562140 to
bf77fd5
Compare
dkropachev
changed the title
Summary
Fixes #940.
Part of #939.
Jira: https://scylladb.atlassian.net/browse/DRIVER-751
Keyspace setup during connection borrow runs an internal
USE "<keyspace>"before the user query is written. Validation failures from thatUSE, such as missing or unauthorized keyspaces, are now surfaced as final query validation errors instead of connection or host failures.Those permanent failures do not defunct the connection, do not try the next host, and do not invoke retry policy callbacks. Driver-side failures and transient server-side failures still follow the existing pre-query next-host path, with pool accounting restored and failed keyspace attempts cleared.
Changes
ConnectionException/BusyConnectionExceptionfromConnection.setKeyspaceAsync()into failed keyspace futures.borrowConnection()reserves a connection but keyspace setup later fails.dequeue(), restoring pending borrow accounting on failure.QueryValidationExceptionfrom internalUSEwithout defuncting the connection.0x2200 INVALIDresponses.setKeyspace()validation errors and busy-connection behavior.Testing
Result: 8 tests, 0 failures, 0 errors.