Allow retry policies to handle client-side request timeouts#960
Open
dkropachev wants to merge 1 commit into
Open
Allow retry policies to handle client-side request timeouts#960dkropachev wants to merge 1 commit into
dkropachev wants to merge 1 commit into
Conversation
|
Warning Review limit reached
Next review available in: 58 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (12)
📝 WalkthroughWalkthroughThe retry API now exposes Sequence Diagram(s)sequenceDiagram
participant Client
participant CqlRequestHandler
participant RetryPolicy
participant Node
Client->>CqlRequestHandler: submit statement
CqlRequestHandler->>Node: send request
CqlRequestHandler->>RetryPolicy: report DriverTimeoutException
RetryPolicy-->>CqlRequestHandler: return RetryVerdict
CqlRequestHandler->>Node: retry on next host when permitted
CqlRequestHandler-->>Client: complete result or timeout error
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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.
Inline comments:
In
`@core/src/main/java/com/datastax/oss/driver/internal/core/cql/CqlRequestHandler.java`:
- Around line 322-334: Update CqlRequestHandler.cancelTimedOutCallbacks and the
corresponding callback-registration path so timeout cancellation and new
callback registration are synchronized or guarded by a timeout generation/state
check. Reject and cancel callbacks created from stale executions after
cancellation, preventing them from running beside the retry; preserve
activeExecutionsCount accuracy when such callbacks are rejected.
- Around line 270-275: The callback-null branch in
CqlRequestHandler.processRequestTimeout currently fails pre-dispatch timeouts
without consulting the retry policy. Preserve or establish the request context
needed by retry handling, and route this path through the same
RetryPolicy.onRequestTimeoutVerdict flow used for in-flight callbacks, allowing
the policy to retry or rethrow instead of calling setFinalError directly.
In
`@core/src/main/java/com/datastax/oss/driver/internal/core/retry/ConsistencyDowngradingRetryPolicy.java`:
- Around line 277-295: Update onRequestTimeoutVerdict so an idempotent request
returns RETRY_NEXT only when retryCount is zero; return RETHROW for subsequent
client-side timeouts while preserving the existing trace logging.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f57070db-8130-41fc-8fb4-4a70a5a09822
📒 Files selected for processing (12)
changelog/README.mdcore/revapi.jsoncore/src/main/java/com/datastax/oss/driver/api/core/retry/RetryPolicy.javacore/src/main/java/com/datastax/oss/driver/internal/core/cql/CqlRequestHandler.javacore/src/main/java/com/datastax/oss/driver/internal/core/retry/ConsistencyDowngradingRetryPolicy.javacore/src/main/java/com/datastax/oss/driver/internal/core/retry/DefaultRetryPolicy.javacore/src/test/java/com/datastax/oss/driver/api/core/retry/ConsistencyDowngradingRetryPolicyTest.javacore/src/test/java/com/datastax/oss/driver/api/core/retry/DefaultRetryPolicyTest.javacore/src/test/java/com/datastax/oss/driver/api/core/retry/RetryPolicyTestBase.javacore/src/test/java/com/datastax/oss/driver/internal/core/cql/CqlRequestHandlerTest.javacore/src/test/java/com/datastax/oss/driver/internal/core/cql/RequestHandlerTestHarness.javamanual/core/retries/README.md
dkropachev
force-pushed
the
dk/issue-958-request-timeout-retry
branch
from
July 16, 2026 12:50
0080f1c to
e90312d
Compare
Author
|
@autopilot review |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
core/src/test/java/com/datastax/oss/driver/internal/core/cql/CqlRequestHandlerTest.java (2)
307-310: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that dispatch remains blocked until the throttler releases it.
An incorrect immediate dispatch could receive node1’s response before
onThrottleReady, yet the final success assertion would still pass.Proposed assertion
CapturedTimeout requestTimeout = harness.nextScheduledTimeout(); requestTimeout.task().run(requestTimeout); + assertThat(resultSetFuture.toCompletableFuture()).isNotDone(); throttledCaptor.getValue().onThrottleReady(true);🤖 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 `@core/src/test/java/com/datastax/oss/driver/internal/core/cql/CqlRequestHandlerTest.java` around lines 307 - 310, Update the test flow around throttledCaptor.getValue().onThrottleReady(true) to assert that no request dispatch or response occurs after the scheduled timeout runs but before the throttler is released. Then retain the release call and existing success assertions to verify dispatch proceeds only after onThrottleReady.
220-221: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise the restarted timeout and incremented retry count.
Every retry currently succeeds immediately, so these tests would still pass if the retry did not schedule a fresh timeout or propagated retry count
0again. Add a hanging second attempt, fire its captured timeout, and verify the policy receives retry count1.🤖 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 `@core/src/test/java/com/datastax/oss/driver/internal/core/cql/CqlRequestHandlerTest.java` around lines 220 - 221, Extend the retry test around requestTimeout.task().run(requestTimeout) to make the second attempt hang instead of succeeding immediately, capture and fire its newly scheduled timeout, and assert the retry policy receives retry count 1. Preserve the existing first-attempt timeout flow while ensuring the test exercises both rescheduling and incremented retry state.
🤖 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.
Inline comments:
In
`@core/src/main/java/com/datastax/oss/driver/internal/core/cql/CqlRequestHandler.java`:
- Around line 319-321: Update the IGNORE handling in CqlRequestHandler,
including the corresponding path around the alternate reported lines, so
RequestTracker.onSuccess is never invoked with a null coordinator. Skip the
success callback when no coordinator is available, or preserve and pass a valid
coordinator context while keeping the existing timeout cancellation and
final-result behavior.
- Around line 156-160: Update the pre-dispatch timeout handling and
onThrottleReady coordination in CqlRequestHandler to synchronize one immutable
phase/state snapshot under the existing lock, rather than independently checking
callback, started, preDispatchStatement, and preDispatchRetryCount. Add an
explicit state indicating that throttle dispatch is still pending, and allow the
timeout transition only from that state; atomically clear or replace the pending
request and retry count so onThrottleReady cannot dispatch the old request
concurrently with the retry.
- Around line 818-819: Update the catch block in the overload that has the
execution parameter to pass that existing execution value to setFinalError
instead of -1. Preserve the current error and node arguments while retaining the
correct execution metadata when finalization fails.
---
Nitpick comments:
In
`@core/src/test/java/com/datastax/oss/driver/internal/core/cql/CqlRequestHandlerTest.java`:
- Around line 307-310: Update the test flow around
throttledCaptor.getValue().onThrottleReady(true) to assert that no request
dispatch or response occurs after the scheduled timeout runs but before the
throttler is released. Then retain the release call and existing success
assertions to verify dispatch proceeds only after onThrottleReady.
- Around line 220-221: Extend the retry test around
requestTimeout.task().run(requestTimeout) to make the second attempt hang
instead of succeeding immediately, capture and fire its newly scheduled timeout,
and assert the retry policy receives retry count 1. Preserve the existing
first-attempt timeout flow while ensuring the test exercises both rescheduling
and incremented retry state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9ad75072-bdf6-42ef-a8ba-d910e649c659
📒 Files selected for processing (12)
changelog/README.mdcore/revapi.jsoncore/src/main/java/com/datastax/oss/driver/api/core/retry/RetryPolicy.javacore/src/main/java/com/datastax/oss/driver/internal/core/cql/CqlRequestHandler.javacore/src/main/java/com/datastax/oss/driver/internal/core/retry/ConsistencyDowngradingRetryPolicy.javacore/src/main/java/com/datastax/oss/driver/internal/core/retry/DefaultRetryPolicy.javacore/src/test/java/com/datastax/oss/driver/api/core/retry/ConsistencyDowngradingRetryPolicyTest.javacore/src/test/java/com/datastax/oss/driver/api/core/retry/DefaultRetryPolicyTest.javacore/src/test/java/com/datastax/oss/driver/api/core/retry/RetryPolicyTestBase.javacore/src/test/java/com/datastax/oss/driver/internal/core/cql/CqlRequestHandlerTest.javacore/src/test/java/com/datastax/oss/driver/internal/core/cql/RequestHandlerTestHarness.javamanual/core/retries/README.md
🚧 Files skipped from review as they are similar to previous changes (6)
- changelog/README.md
- core/src/test/java/com/datastax/oss/driver/api/core/retry/RetryPolicyTestBase.java
- core/src/main/java/com/datastax/oss/driver/api/core/retry/RetryPolicy.java
- manual/core/retries/README.md
- core/src/main/java/com/datastax/oss/driver/internal/core/retry/ConsistencyDowngradingRetryPolicy.java
- core/src/main/java/com/datastax/oss/driver/internal/core/retry/DefaultRetryPolicy.java
dkropachev
force-pushed
the
dk/issue-958-request-timeout-retry
branch
from
July 16, 2026 14:10
e90312d to
f74f8dc
Compare
dkropachev
force-pushed
the
dk/issue-958-request-timeout-retry
branch
from
July 16, 2026 14:10
f74f8dc to
8df5122
Compare
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
1 participant
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.
Fixes #958
Summary
RetryPolicy.onRequestTimeoutVerdict(...)for client-sideDriverTimeoutExceptiondecisions with resolved consistency, idempotence, and retry countCqlRequestHandlerrequest-timeout handling through the retry policy; retry verdicts cancel timed-out in-flight callbacks and restart the request timeout for the retried requestDefaultRetryPolicyconservative by rethrowing; letConsistencyDowngradingRetryPolicyretry idempotent request timeouts on the next nodeTesting
JAVA_HOME=/usr/lib/jvm/java-17-openjdk-amd64 PATH=/usr/lib/jvm/java-17-openjdk-amd64/bin:$PATH mvn -pl core -am -DskipTests test-compileJAVA_HOME=/usr/lib/jvm/java-17-openjdk-amd64 PATH=/usr/lib/jvm/java-17-openjdk-amd64/bin:$PATH mvn -pl core -am -Dtest=CqlRequestHandlerTest,DefaultRetryPolicyTest,ConsistencyDowngradingRetryPolicyTest -Dsurefire.failIfNoSpecifiedTests=false testJAVA_HOME=/usr/lib/jvm/java-17-openjdk-amd64 PATH=/usr/lib/jvm/java-17-openjdk-amd64/bin:$PATH mvn -pl core -am -DskipTests -DskipITs verify