feat: auto retry on deployment-related 499 errors #240
Conversation
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
Moved retryOnException to a separate file and refactored it to return the result directly instead of a Future. Updated ApiClientImpl to use the new retryOnException implementation and simplified the getEvaluations and registerEvents methods accordingly.
Added comprehensive tests for the registerEvents API client method, covering success, various error codes, retry logic for 499 errors, and ensuring no retries for other 3xx, 4xx, and 5xx errors. This improves test coverage and verifies correct retry and error handling behavior.
Renamed retryOnExceptionSync to retryOnException and updated all usages in implementation and tests. Adjusted retry logic to fix off-by-one error in maxAttempts. Renamed test file and test class to match the new function name.
Commented out the CLIENT_CLOSED_REQUEST test case since it has special retry handling. The relevant test can be found in ApiClientRetryTest.kt.
Adjusted spacing in comments for consistency in ApiClientImplTest.kt. No functional changes were made.
There was a problem hiding this comment.
Pull Request Overview
This PR adds retry logic specifically for handling 499 (Client Closed Request) errors in API calls. The implementation introduces a generic retryOnException utility function that retries requests up to 3 times when encountering 499 status codes, while immediately failing for all other error types.
- Created a new
retryOnExceptionutility function with configurable retry logic - Updated
getEvaluationsandregisterEventsmethods to wrap API calls with retry logic for 499 errors - Added comprehensive test coverage in
ApiClientImplRetryTest.ktandRetryOnExceptionTest.kt
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| RetryOnException.kt | New utility function for retrying operations on specific exceptions with exponential backoff |
| ApiClientImpl.kt | Integrated retry logic into getEvaluations and registerEvents to handle 499 errors |
| ApiClientImplRetryTest.kt | Comprehensive test suite covering retry behavior for both API methods with various error scenarios |
| RetryOnExceptionTest.kt | Unit tests for the retryOnException utility function covering edge cases |
| ApiClientImplTest.kt | Commented out 499 test case and added placeholder test stubs referencing the new test file |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
bucketeer/src/main/kotlin/io/bucketeer/sdk/android/internal/remote/RetryOnException.kt
Outdated
Show resolved
Hide resolved
bucketeer/src/main/kotlin/io/bucketeer/sdk/android/internal/remote/RetryOnException.kt
Show resolved
Hide resolved
Changed the retry loop from '0..maxAttempts' to '0 until maxAttempts' to ensure the number of attempts matches the intended maxRetries logic and avoids an off-by-one error.
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.
bucketeer/src/main/kotlin/io/bucketeer/sdk/android/internal/remote/ApiClientImpl.kt
Outdated
Show resolved
Hide resolved
bucketeer/src/main/kotlin/io/bucketeer/sdk/android/internal/remote/RetryOnException.kt
Show resolved
Hide resolved
bucketeer/src/main/kotlin/io/bucketeer/sdk/android/internal/remote/RetryOnException.kt
Show resolved
Hide resolved
bucketeer/src/main/kotlin/io/bucketeer/sdk/android/internal/remote/RetryOnException.kt
Show resolved
Hide resolved
bucketeer/src/main/kotlin/io/bucketeer/sdk/android/internal/remote/RetryOnException.kt
Show resolved
Hide resolved
Added detailed KDoc and inline comments to the retryOnException function, clarifying its usage, parameters, and behavior. This improves code readability and helps developers understand the retry logic and threading considerations.
Corrected the KDoc comment formatting by removing extra indentation to ensure proper documentation rendering.
duyhungtnn
changed the title
Collaborator
Author
|
hi @cre8ivejp please help me to take a look |
Added two tests to verify fetchEvaluations retries on HTTP 499 responses and handles failure after multiple retries. The tests check correct evaluation storage updates, event logging, and request count to ensure proper retry and error handling behavior.
Requests are now cloned using newBuilder().build() before being passed to newCall, preventing issues related to reusing the same request instance in OkHttp.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
bucketeer/src/main/kotlin/io/bucketeer/sdk/android/internal/remote/RetryOnException.kt
Show resolved
Hide resolved
Updated the KDoc for RetryOnException to specify that a linear backoff delay is applied between retries, improving clarity for users of the function.
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.
refs: bucketeer-io/javascript-client-sdk#265
This pull request introduces a new retry mechanism for API requests in the Bucketeer Android SDK, specifically targeting cases where a
ClientClosedRequestException(HTTP 499) occurs. The retry logic is encapsulated in a reusable function and is thoroughly unit tested. Additionally, the main API client logic is refactored to use this retry utility, and related test coverage is updated.Retry mechanism and API client improvements:
retryOnExceptionutility function inRetryOnException.ktthat retries a block of code on specified exceptions, with configurable retry count and delay.ApiClientImplto useretryOnExceptionfor API calls, specifically retrying up to 3 times with exponential delay when aBKTException.ClientClosedRequestException(HTTP 499) is encountered. [1] [2] [3] [4]BKTExceptioninApiClientImpl.ktto support the new retry logic.Testing and test coverage:
retryOnExceptioninRetryOnExceptionTest.kt, covering success, failure, non-retriable exceptions, and value propagation.ApiClientImplTest.ktto skip direct testing of HTTP 499 retry logic (now handled in a dedicated test), and added placeholders for new retry-related tests. [1] [2]