RetryingHttpRequesterFilter: add on request retry callback
#2916
Conversation
Motivation: There are use-cases when users need to perform an action before the retry, like updating meta-data or logging/metrics. Because all retry functions return `BackoffPolicy`, it's still not known inside the function of `BackoffPolicy` will retry or not because of the retry counts limit. Modifications: - Add `RetryingHttpRequesterFilter.Builder.onRequestRetry(BiConsumer)` that users can use to intercept every retry attempt; - Test that the new callback works for request retries; Result: Users can see when the retry actually happens, after backoff time. If there will be a similar use-case for `reserveConnection` in the future, we will add a separate callback.
| * {@link StreamingHttpClient#request(StreamingHttpRequest) request} retry attempt | ||
| * @return {@code this} | ||
| */ | ||
| public Builder onRequestRetry(final BiConsumer<HttpRequestMetaData, Throwable> onRequestRetry) { |
There was a problem hiding this comment.
Would it be useful if the number of retries on the current attempt are also made available to the consumer somehow? I know one can do this manually by setting it on the context and tracking it on their own, but I would assume that the number of attempts is something a lot of people need, if only for logging.
There was a problem hiding this comment.
Good point. I will have to come up with a new interface. Will do that, thanks!
There was a problem hiding this comment.
Any suggestion for the name? Assuming we may need a similar feature for reserveConnection retries
There was a problem hiding this comment.
Its not very apparent from the name at which stage the flow runs other than the javadoc. WDYT about including such information as part of the name? Something like onBeforeRequestRetry
There was a problem hiding this comment.
Regarding the interface my 2c would be BeforeRetryConsumer or BeforeRetryInfluencer to highlight the ability to change things in the flow.
There was a problem hiding this comment.
I will probably go with beforeRequestRetry(BeforeRetryCallback) if no objections
There was a problem hiding this comment.
I ended up keeping the builder name as-is, but clarifying "before" as part of the RetryCallbacks contract.
Naming is hard, open to other suggestions in the context of a new interface.
servicetalk-http-netty/src/main/java/io/servicetalk/http/netty/RetryingHttpRequesterFilter.java
Outdated
Show resolved
Hide resolved
servicetalk-http-netty/src/main/java/io/servicetalk/http/netty/RetryingHttpRequesterFilter.java
Show resolved
Hide resolved
servicetalk-http-netty/src/main/java/io/servicetalk/http/netty/RetryingHttpRequesterFilter.java
Show resolved
Hide resolved
servicetalk-http-netty/src/main/java/io/servicetalk/http/netty/RetryingHttpRequesterFilter.java
Outdated
Show resolved
Hide resolved
Motivation:
There are use-cases when users need to perform an action before the retry, like updating meta-data or logging/metrics. Because all retry functions return
BackoffPolicy, it's still not known inside the function ofBackoffPolicywill retry or not because of the retry counts limit.Modifications:
RetryingHttpRequesterFilter.Builder.onRequestRetry(BiConsumer)that users can use to intercept every retry attempt;Result:
Users can see when the retry actually happens, after backoff time.
If there will be a similar use-case for
reserveConnectionin the future, we will add a separate callback.