Add perMethod, perHost, perPath and builder to RetryConfigMapping

[schiemon]

Motivation:

The motivation of this PR is explained in the associated issue.

Modifications:

• Added perMethod, perHost, perPath and perHostAndMethod to RetryConfigMapping to enable retry configuration on a per method, host and path basis. This API is analogous to CircuitBreakerMapping.
• Added RetryConfig.noRetry for being more concise when signalling that no retry is expected, especially in the per*methods.
• Also added RetryConfigMapping.builder and RetryConfigMappingBuilder. This API is analogous to CircuitBreakerMappingBuilder.
• Moved common utility methods from CircuitBreakerMappingUtil to RequestContextUtil
• Added (missing) tests for RetryConfigMapping

Result:

• Closes Enable creating a RetryConfigMapping on a per method, path, or host basis #6242
• RetryConfigMapping.of is deprecated.

[CLAassistant]

All committers have signed the CLA.

[github-actions]

🔍 Build Scan® (commit: cb5fe28)

Job name	Status	Build Scan®
build-ubicloud-standard-16-jdk-8	✅	https://ge.armeria.dev/s/egnyw65pee6fi
build-ubicloud-standard-16-jdk-21-snapshot-blockhound	✅	https://ge.armeria.dev/s/g4lp5nupw4pte
build-ubicloud-standard-16-jdk-17-min-java-17-coverage	✅	https://ge.armeria.dev/s/jkrpy4sz5qzn4
build-ubicloud-standard-16-jdk-17-min-java-11	✅	https://ge.armeria.dev/s/lsfju34zhlhqc
build-ubicloud-standard-16-jdk-17-leak	✅	https://ge.armeria.dev/s/qausxqm4qvcto
build-ubicloud-standard-16-jdk-11	✅	https://ge.armeria.dev/s/fu5gliz64rgp4
build-macos-latest-jdk-21	✅	https://ge.armeria.dev/s/kvzlbo5f72eau

[schiemon]

After the main part of the review I will add the javadocs :)

[codecov]

Codecov Report

❌ Patch coverage is 98.27586% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 74.69%. Comparing base (8150425) to head (cb5fe28).
⚠️ Report is 477 commits behind head on main.

Files with missing lines	Patch %	Lines
...rp/armeria/internal/common/RequestContextUtil.java	90.90%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #6243      +/-   ##
============================================
+ Coverage     74.46%   74.69%   +0.23%     
- Complexity    22234    22546     +312     
============================================
  Files          1963     1976      +13     
  Lines         82437    83184     +747     
  Branches      10764    10821      +57     
============================================
+ Hits          61385    62135     +750     
+ Misses        15918    15897      -21     
- Partials       5134     5152      +18     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[jrhee17]

I do think RetryConfig#noRetry will be useful, and I also appreciate the extensive test coverage.
Left a comment on APIs added to RetryConfigMapping

[jrhee17]

Thanks for the initiative. I agree intuitively, it makes sense that retry and circuitbreaker share similar APIs.

Having said this, I do think retry is a little different from circuitbreaker in the sense that CircuitBreaker share a common mutable configuration across requests. For this reason, a mapping structure is used to cache the CircuitBreakers used per host/method/path.

On the other hand, RetryConfig is an immutable object which decides how each request will be retried (in case there is a retry). Hence, there is no real reason we should be caching the returned RetryConfigs. It may be worth checking the discussion at #3145 as well.

I do think there is value in pre-parsing method/host/path parameters so that users don't have to extract each value from the ctx though.

e.g.

static <T extends Response> RetryConfigMapping<T> of(RetryConfigFactory<T> retryConfigFactory) {
}

I'm not sure of the usefulness of the builder() or per*() apis though since users can simply choose to ignore certain fields of the RetryConfigFactory anyways.

e.g. users can simply choose not to use the path

RetryConfigMapping.of((host, method, path) -> {
    if ("asdf".equals(host)) {
        return RetryConfig.noRetry();   
    }
    ...
});

Let me know what you think.

cc. other maintainers @line/dx

[schiemon]

Thanks for your review @jrhee17.

On the other hand, RetryConfig is an immutable object which decides how each request will be retried (in case there is a retry). Hence, there is no real reason we should be caching the returned RetryConfigs.

Ah, so you're effectively saying that RetryConfigMapping doesn't necessarily need a keyFactory in RetryConfigMapping.of:

static <T extends Response> RetryConfigMapping<T> of(
        BiFunction<? super ClientRequestContext, Request, String> keyFactory, // Could be removed and we could just recreate the `RetryConfig`s for every request as they are immutable.
        BiFunction<? super ClientRequestContext, Request, RetryConfig<T>> retryConfigFactory) {
    return new KeyedRetryConfigMapping<>(keyFactory, retryConfigFactory);
}

and that we could offer an "uncached" version like:

// Returns a mapping that regenerates the `RetryConfig` on every request. Would not work with
// `CircuitBreaker`s, as multiple requests hitting the same host may need to mutate the same
// `CircuitBreaker`.
static <T extends Response> RetryConfigMapping<T> of(
        BiFunction<? super ClientRequestContext, Request, RetryConfig<T>> retryConfigFactory) {
    return new KeyedRetryConfigMapping<>(retryConfigFactory);
}

Since this API is not currently offered, and because RetryConfigMapping mentions caching twice, I assumed there was a performance reason for caching — for example, reducing GC pressure or handling expensive factory methods.

If it’s just for ergonomics, I like your suggestion as it avoids specifying the keyFactory. However, given that we already cache in KeyedRetryConfigMapping, and considering that we have CircuitBreakerMapping alongside it, I would lean towards using a per*-style approach.

I'm also happy to hear input from the other maintainers.