MLE-27388 Refactor: Cutting down on duplicated retry code#1914
MLE-27388 Refactor: Cutting down on duplicated retry code#1914
Conversation
|
Copyright Validation Results ✅ Valid Files
✅ All files have valid copyright headers! |
|
Closing, just wanted to see the tests pass. |
Added RetryContext and tossed in some debug logging, but that may change a bit in the next PR. This gets rid of 9 occurrences of the same code and also means that logging for the retry-50x can be done in a single place now.
rjrudin
force-pushed
the
feature/27388-retry
branch
from
603153e to
e8642cb
Compare
rjrudin
changed the title
There was a problem hiding this comment.
Pull request overview
This PR introduces a small retry helper to reduce duplicated retry-loop code in OkHttpServices, with the goal of centralizing retry state (attempt count, elapsed time, and delay calculation) so it’s easier to add consistent debug logging later.
Changes:
- Added a new
RetryContexthelper to track retry attempt count, elapsed time, and next sleep delay. - Refactored multiple retry loops in
OkHttpServicesto useRetryContextinstead of localstartTime/nextDelay/retryvariables. - Centralized the “max retries exceeded” exception construction (and
checkFirstRequestcallback) intoRetryContext.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
marklogic-client-api/src/main/java/com/marklogic/client/impl/RetryContext.java |
New helper class encapsulating retry loop state, sleep, delay computation, and max-retry failure throwing. |
marklogic-client-api/src/main/java/com/marklogic/client/impl/OkHttpServices.java |
Replaces repeated retry-loop state variables with RetryContext across multiple HTTP operation implementations. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| void throwIfMaxRetriesExceeded(int status) { | ||
| if (retryableStatusCodes.contains(status)) { | ||
| if (onMaxRetriesCallback != null) { | ||
| onMaxRetriesCallback.run(); | ||
| } |
There was a problem hiding this comment.
throwIfMaxRetriesExceeded is named as if it always throws, but it silently no-ops when status is not in retryableStatusCodes. This makes the method easy to misuse in the future (callers may assume it always throws). Consider either removing the internal status check (and always throwing) or renaming the method to reflect the conditional behavior (and/or returning a boolean indicating whether it threw).
| try { | ||
| retryContext.sleepIfNeeded(); | ||
| } catch (InterruptedException e) { | ||
| // Ignore interruption |
There was a problem hiding this comment.
Catching InterruptedException and ignoring it clears the thread's interrupted status, which can make it harder for callers/executors to stop work promptly. Consider restoring the interrupt flag via Thread.currentThread().interrupt() (and ideally exiting the retry loop) so interruption can be honored.
| // Ignore interruption | |
| Thread.currentThread().interrupt(); | |
| break; |
Had Copilot add a RetryContext to get rid of a bunch of duplication (in 9 places) so that debug logging can then be added.
Not merging this yet, just want to see the tests run first.