Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow user-configurable retry delay and check #877

Closed
wants to merge 2 commits into from
Closed

Conversation

quinnj
Copy link
Member

@quinnj quinnj commented Jul 12, 2022

It's been reported that users sometimes feel the need to disable HTTP's
retry abilities and wrap HTTP methods with their own retry logic due
to the lack of configurability around delays and overriding the retry
check. Since there is some tricky logic in getting retries just right,
especially in the presence of streaming request or response bodies,
this PR proposes more configurability for retry logic in the hopes
that users won't opt to roll their own incorrect retry logic.

It's been reported that users sometimes feel the need to disable HTTP's
retry abilities and wrap HTTP methods with their own retry logic due
to the lack of configurability around delays and overriding the retry
check. Since there is some tricky logic in getting retries just right,
especially in the presence of streaming request or response bodies,
this PR proposes more configurability for retry logic in the hopes
that users won't opt to roll their own incorrect retry logic.
@codecov-commenter
Copy link

codecov-commenter commented Jul 12, 2022

Codecov Report

Merging #877 (e72faa2) into master (98a0b69) will increase coverage by 0.02%.
The diff coverage is 85.71%.

@@            Coverage Diff             @@
##           master     #877      +/-   ##
==========================================
+ Coverage   80.01%   80.03%   +0.02%     
==========================================
  Files          36       36              
  Lines        2867     2870       +3     
==========================================
+ Hits         2294     2297       +3     
  Misses        573      573              
Impacted Files Coverage Δ
src/clientlayers/RetryRequest.jl 75.55% <50.00%> (-1.19%) ⬇️
src/Messages.jl 87.65% <100.00%> (+0.69%) ⬆️
src/clientlayers/StreamRequest.jl 97.14% <100.00%> (ø)

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@quinnj
Copy link
Member Author

quinnj commented Jul 13, 2022

Ok, challenges here:

  • We currently check retryable(stream) in readbody to see if we should write the response body out to a user-provided response stream or "save it" temporarily in the request context
  • But if we let the user provide the retry_check, then that needs to also be factored into the retryable(stream) check
  • But I would expect, as the user, for the retry_check interface to be something like f(req, resp, body, ex) -> Bool where I'm indicating, for a given request/response/body/exception whether the request should be retried or not.

So I'm not quite sure what to do. There are valid cases (like here), where you might want to read the response body before deciding whether to do a retry. But how do we make that work if you provided a response_stream?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants