refactor: always attemp to reset readers during retry#7
Merged
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR simplifies the retry mechanism by making reader reset functionality unconditional, removing the SetRetryResetReaders API and related configuration. The change makes all io.ReadSeeker implementations (including multipart fields and request bodies) reset automatically on retry attempts.
- Removed
SetRetryResetReaders()method andRetryResetReadersfield from the Client - Made reader reset behavior unconditional in the retry logic
- Refactored
createHTTPRequestto usehttp.NewRequestWithContextdirectly instead of callingWithContextseparately - Increased test tolerance for timing precision from 1ms to 100ms
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| client.go | Removed RetryResetReaders field and SetRetryResetReaders() method |
| retry.go | Removed ResetMultipartReaders() option function and made reader reset unconditional, removed unused resetReaders field from Options struct |
| request.go | Removed ResetMultipartReaders option from retry configuration |
| middleware.go | Refactored to use http.NewRequestWithContext and moved trace initialization before request creation |
| retry_test.go | Removed SetRetryResetReaders(true) calls from tests and increased timing tolerance to 100ms |
| request_test.go | Removed SetRetryResetReaders(true) call from test |
| multipart_test.go | Removed SetRetryResetReaders(true) call from test |
Comments suppressed due to low confidence (1)
retry.go:47
- The
resetReadersfield is no longer used in the Options struct after removing theResetMultipartReaders()function, but it still exists in the struct definition. This unused field should be removed to maintain code cleanliness.
resetReaders bool
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Zack Zeng <zackzeng96@gmail.com>
42d980e to
0886b1c
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
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.
No description provided.