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

Configurable request retry #2453

Closed

Conversation

crankydillo
Copy link

@crankydillo crankydillo commented Aug 27, 2022

Allow more configuration when it comes to retrying requests.

This is currently failing at least 1 test for 2 reasons that need to be addressed. First, Retry#max exhaustion generates the retryexhausted wrapper. I think that's a good thing, but I understand why you'd go a different route from multiple perspectives (IO checked retry, breaking behavior). Still thinking on that, but feel free to tell me what to do.

Also, in the error case, this fires the doOnError hook maxRetry + 1. Still looking into this.

Need to stop for the day, but figured I'd get a draft out there. I'll clean up all commits/code/etc. before moving out of draft.

#2425

Samuel Cox and others added 3 commits August 27, 2022 06:57
The main issue we can't use generic reactor-core retry concept is due to
the inability to detect if HTTP-level data has been sent across the
wire.

 reactor#2425
@pivotal-cla
Copy link

@crankydillo Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@violetagg
Copy link
Member

@crankydillo I'm gonna check this next week

@crankydillo
Copy link
Author

Thanks Violeta. Honestly, I need to work on it. It was not as easy as initially thought:) I can pass all the tests with my current code (not pushed yet), but I'm not happy about it, and I think there may be some corner cases I'm missing. I'll do my best to work on it a bit over the next week.

@crankydillo
Copy link
Author

I pushed it, but I'm not happy about it. It's going to be a busy week or two, so I'm not sure when I will come back to it. Bottom line is I couldn't figure out how to avoid some huge redesign to the observers and still do this in a clean way. The crux of the problem revolves around getting that done-retrying signal to the observers at the right time. The only other idea I have is to have an internal, failing always sink-based flux that fails always embedded within HttpObserver, but that feels hacky, generates a bunch of unnecessary exceptions, and still has some problems. The main one being not knowing exactly what prompts retry (since its configurable!).

@crankydillo
Copy link
Author

On a side note, I thought I'd already signed the Pivotal agreement. I'll get it signed again I guess.

@crankydillo
Copy link
Author

@violetagg I'm just going to close this. I understand the desire to leverage reactor's retry, but I don't see how that can quickly be done. Given that my team is not demanding this feature, I doubt I'll be able to resolve this.

@crankydillo crankydillo closed this Oct 3, 2022
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.

3 participants