Re-sign request on retry #216
Conversation
|
|
|
Hello @dekimsey and @emilymianeil , do you have any timeline when this could be reviewed and merged? |
There was a problem hiding this comment.
Thank you @mgwoj for this PR! This seems like a reasonable addition and I like the implementation. My only suggestion would be to accommodate a default nil value for Client.PrepareRetry - this would offer maximum compatibility for consumers who might be constructing their own Client.
client.go
Outdated
| @@ -435,6 +441,7 @@ func NewClient() *Client { | |||
| RetryMax: defaultRetryMax, | |||
| CheckRetry: DefaultRetryPolicy, | |||
| Backoff: DefaultBackoff, | |||
| PrepareRetry: DefaultPrepareRetry, | |||
There was a problem hiding this comment.
Since this default function does nothing, can we leave the default as nil here?
client.go
Outdated
| // DefaultPrepareRetry is performing noop during prepare retry | ||
| func DefaultPrepareRetry(_ *http.Request) error { | ||
| // noop | ||
| return nil | ||
| } | ||
|
|
There was a problem hiding this comment.
Not needed if we accommodate a nil value for client.PrepareRetry
client.go
Outdated
| if err := c.PrepareRetry(req.Request); err != nil { | ||
| prepareErr = err | ||
| break | ||
| } |
There was a problem hiding this comment.
Per the above, can we conditionally attempt this if the function is non-nil?
| if err := c.PrepareRetry(req.Request); err != nil { | |
| prepareErr = err | |
| break | |
| } | |
| if c.PrepareRetry != nil { | |
| if err := c.PrepareRetry(req.Request); err != nil { | |
| prepareErr = err | |
| break | |
| } | |
| } |
|
Thank you for reviewing and accepting the proposed solution for prepare retry. I have applied your suggestions. |
In our case we need to re-sign request before initiating retry operation. To achieve that we have added a new handler
PrepareRetrywhich can be used for this purpose, however the signature and the name is as generic as possible.