Skip to content

Add perMethod, perHost, perPath and builder to RetryConfigMapping #6243

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.

Sign up for GitHub

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Add perMethod, perHost, perPath and builder to RetryConfigMapping #6243

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b63e552
feat: add perMethod, perHost, perPath to RetryConfigMapping
schiemon May 17, 2025
087b025
test: add tests for RetryConfigMapping
schiemon May 17, 2025
bc52c44
refactor: revert deprecation of `RetryConfigMapping.of`
schiemon May 19, 2025
cb5fe28
feat: add `RetryConfig.noRetry()` and add tests
schiemon May 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions .../src/main/java/com/linecorp/armeria/client/circuitbreaker/KeyedCircuitBreakerMapping.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/*
* Copyright 2016 LINE Corporation
* Copyright 2025 LY Corporation
*
* LINE Corporation licenses this file to you under the Apache License,
* LY Corporation licenses this file to you under the Apache License,
* version 2.0 (the "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at:
*
Expand All @@ -16,9 +16,9 @@

package com.linecorp.armeria.client.circuitbreaker;

import static com.linecorp.armeria.internal.common.circuitbreaker.CircuitBreakerMappingUtil.host;
import static com.linecorp.armeria.internal.common.circuitbreaker.CircuitBreakerMappingUtil.method;
import static com.linecorp.armeria.internal.common.circuitbreaker.CircuitBreakerMappingUtil.path;
import static com.linecorp.armeria.internal.common.RequestContextUtil.host;
import static com.linecorp.armeria.internal.common.RequestContextUtil.method;
import static com.linecorp.armeria.internal.common.RequestContextUtil.path;
import static java.util.Objects.requireNonNull;
import static java.util.stream.Collectors.joining;

Expand Down
31 changes: 29 additions & 2 deletions core/src/main/java/com/linecorp/armeria/client/retry/KeyedRetryConfigMapping.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/*
* Copyright 2020 LINE Corporation
* Copyright 2025 LY Corporation
*
* LINE Corporation licenses this file to you under the Apache License,
* LY Corporation licenses this file to you under the Apache License,
* version 2.0 (the "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at:
*
Expand All @@ -16,11 +16,17 @@

package com.linecorp.armeria.client.retry;

import static com.linecorp.armeria.internal.common.RequestContextUtil.host;
import static com.linecorp.armeria.internal.common.RequestContextUtil.method;
import static com.linecorp.armeria.internal.common.RequestContextUtil.path;
import static java.util.Objects.requireNonNull;
import static java.util.stream.Collectors.joining;

import java.util.Objects;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.ConcurrentMap;
import java.util.function.BiFunction;
import java.util.stream.Stream;

import com.linecorp.armeria.client.ClientRequestContext;
import com.linecorp.armeria.common.Request;
Expand All @@ -38,6 +44,27 @@ final class KeyedRetryConfigMapping<T extends Response> implements RetryConfigMa
this.retryConfigFactory = requireNonNull(retryConfigFactory, "retryConfigFactory");
}

KeyedRetryConfigMapping(
boolean perHost, boolean perMethod, boolean perPath, RetryConfigFactory retryConfigFactory) {
requireNonNull(retryConfigFactory, "retryConfigFactory");

keyFactory = (ctx, req) -> {
final String host = perHost ? host(ctx) : null;
final String method = perMethod ? method(ctx) : null;
final String path = perPath ? path(ctx) : null;
return Stream.of(host, method, path)
.filter(Objects::nonNull)
.collect(joining("#"));
};

this.retryConfigFactory = (ctx, req) -> {
final String host = perHost ? host(ctx) : null;
final String method = perMethod ? method(ctx) : null;
final String path = perPath ? path(ctx) : null;
return retryConfigFactory.apply(host, method, path);
};
}

@Override
public RetryConfig<T> get(ClientRequestContext ctx, Request req) {
final String key = keyFactory.apply(ctx, req);
Expand Down
13 changes: 11 additions & 2 deletions core/src/main/java/com/linecorp/armeria/client/retry/RetryConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/*
* Copyright 2020 LINE Corporation
* Copyright 2025 LY Corporation
*
* LINE Corporation licenses this file to you under the Apache License,
* LY Corporation licenses this file to you under the Apache License,
* version 2.0 (the "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at:
*
Expand All @@ -26,6 +26,7 @@
import com.linecorp.armeria.common.Response;
import com.linecorp.armeria.common.RpcResponse;
import com.linecorp.armeria.common.annotation.Nullable;
import com.linecorp.armeria.common.util.UnmodifiableFuture;

/**
* Holds retry config used by a {@link RetryingClient}.
Expand All @@ -36,6 +37,14 @@ public final class RetryConfig<T extends Response> {

private static final Logger logger = LoggerFactory.getLogger(RetryConfig.class);

private static final RetryConfig<?> NO_RETRY_CONFIG = builder(
(ctx, cause) -> UnmodifiableFuture.completedFuture(RetryDecision.noRetry())).build();

@SuppressWarnings("unchecked")
public static <T extends Response> RetryConfig<T> noRetry() {
return (RetryConfig<T>) NO_RETRY_CONFIG;
}

/**
* Returns a new {@link RetryConfigBuilder} with the specified {@link RetryRule}.
*/
Expand Down
19 changes: 10 additions & 9 deletions ...l/common/circuitbreaker/package-info.java → ...eria/client/retry/RetryConfigFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/*
* Copyright 2022 LINE Corporation
* Copyright 2025 LY Corporation
*
* LINE Corporation licenses this file to you under the Apache License,
* LY Corporation licenses this file to you under the Apache License,
* version 2.0 (the "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at:
*
Expand All @@ -14,11 +14,12 @@
* under the License.
*/

/**
* CircuitBreaker related classes used internally.
* Anything in this package can be changed or removed at any time.
*/
@NonNullByDefault
package com.linecorp.armeria.internal.common.circuitbreaker;
package com.linecorp.armeria.client.retry;

import com.linecorp.armeria.common.Response;
import com.linecorp.armeria.common.annotation.Nullable;

import com.linecorp.armeria.common.annotation.NonNullByDefault;
@FunctionalInterface
public interface RetryConfigFactory<T extends Response> {
RetryConfig<T> apply(@Nullable String host, @Nullable String method, @Nullable String path);
}
43 changes: 40 additions & 3 deletions core/src/main/java/com/linecorp/armeria/client/retry/RetryConfigMapping.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/*
* Copyright 2020 LINE Corporation
* Copyright 2025 LY Corporation
*
* LINE Corporation licenses this file to you under the Apache License,
* LY Corporation licenses this file to you under the Apache License,
* version 2.0 (the "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at:
*
Expand All @@ -16,15 +16,18 @@

package com.linecorp.armeria.client.retry;

import static java.util.Objects.requireNonNull;

import java.util.function.BiFunction;
import java.util.function.Function;

import com.linecorp.armeria.client.ClientRequestContext;
import com.linecorp.armeria.common.Request;
import com.linecorp.armeria.common.Response;

/**
* Returns a {@link RetryConfig} given the {@link ClientRequestContext}.
* Allows users to change retry behavior according to any context element, like host, method, path ...etc.
* Allows users to change retry behavior according to any context element, like host, method, path, etc.
*/
@FunctionalInterface
public interface RetryConfigMapping<T extends Response> {
Expand Down Expand Up @@ -56,6 +59,40 @@ static <T extends Response> RetryConfigMapping<T> of(
return new KeyedRetryConfigMapping<>(keyFactory, retryConfigFactory);
}

static <T extends Response> RetryConfigMappingBuilder<T> builder() {
return new RetryConfigMappingBuilder<>();
}
Comment on lines +62 to +64
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@schiemon schiemon May 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


static <T extends Response> RetryConfigMapping<T> perMethod(Function<String, RetryConfig<T>> factory) {
requireNonNull(factory, "factory");
return RetryConfigMapping.<T>builder()
.perMethod()
.build((host, method, path) -> factory.apply(method));
}

static <T extends Response> RetryConfigMapping<T> perHost(Function<String, RetryConfig<T>> factory) {
requireNonNull(factory, "factory");
return RetryConfigMapping.<T>builder()
.perHost()
.build((host, method, path) -> factory.apply(host));
}

static <T extends Response> RetryConfigMapping<T> perPath(Function<String, RetryConfig<T>> factory) {
requireNonNull(factory, "factory");
return RetryConfigMapping.<T>builder()
.perPath()
.build((host, method, path) -> factory.apply(path));
}

static <T extends Response> RetryConfigMapping<T> perHostAndMethod(
BiFunction<String, String, RetryConfig<T>> factory) {
requireNonNull(factory, "factory");
return RetryConfigMapping.<T>builder()
.perHost()
.perMethod()
.build((host, method, path) -> factory.apply(host, method));
}

/**
* Returns the {@link RetryConfig} that maps to the given {@link ClientRequestContext} and {@link Request}.
*/
Expand Down
55 changes: 55 additions & 0 deletions core/src/main/java/com/linecorp/armeria/client/retry/RetryConfigMappingBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
/*
* Copyright 2025 LY Corporation
*
* LY Corporation licenses this file to you under the Apache License,
* version 2.0 (the "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at:
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
* WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
* License for the specific language governing permissions and limitations
* under the License.
*/

package com.linecorp.armeria.client.retry;

import com.linecorp.armeria.common.Response;

public final class RetryConfigMappingBuilder<T extends Response> {
private boolean perHost;
private boolean perMethod;
private boolean perPath;

public RetryConfigMappingBuilder<T> perHost() {
perHost = true;
return this;
}

public RetryConfigMappingBuilder<T> perMethod() {
perMethod = true;
return this;
}

public RetryConfigMappingBuilder<T> perPath() {
perPath = true;
return this;
}

private boolean validateMappingKeys() {
return perHost || perMethod || perPath;
}

public RetryConfigMapping<T> build(RetryConfigFactory<T> retryConfigFactory) {
if (!validateMappingKeys()) {
throw new IllegalStateException(
"A RetryConfigMapping created by this builder must be per host, method and/or path");
}

return new KeyedRetryConfigMapping<>(
perHost, perMethod, perPath, retryConfigFactory
);
}
}
31 changes: 29 additions & 2 deletions core/src/main/java/com/linecorp/armeria/internal/common/RequestContextUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/*
* Copyright 2019 LINE Corporation
* Copyright 2025 LY Corporation
*
* LINE Corporation licenses this file to you under the Apache License,
* LY Corporation licenses this file to you under the Apache License,
* version 2.0 (the "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at:
*
Expand Down Expand Up @@ -30,12 +30,15 @@
import com.google.common.collect.MapMaker;
import com.google.errorprone.annotations.MustBeClosed;

import com.linecorp.armeria.client.ClientRequestContext;
import com.linecorp.armeria.client.Endpoint;
import com.linecorp.armeria.common.ContextHolder;
import com.linecorp.armeria.common.Flags;
import com.linecorp.armeria.common.HttpRequest;
import com.linecorp.armeria.common.RequestContext;
import com.linecorp.armeria.common.RequestContextStorage;
import com.linecorp.armeria.common.RequestContextStorageProvider;
import com.linecorp.armeria.common.RpcRequest;
import com.linecorp.armeria.common.annotation.Nullable;
import com.linecorp.armeria.common.util.SafeCloseable;
import com.linecorp.armeria.common.util.Sampler;
Expand Down Expand Up @@ -267,5 +270,29 @@ public static void ensureSameCtx(RequestContext ctx, ContextHolder contextHolder
}
}

public static String host(ClientRequestContext ctx) {
final Endpoint endpoint = ctx.endpoint();
if (endpoint == null) {
return "UNKNOWN";
} else {
final String ipAddr = endpoint.ipAddr();
if (ipAddr == null || endpoint.isIpAddrOnly()) {
return endpoint.authority();
} else {
return endpoint.authority() + '/' + ipAddr;
}
}
}

public static String method(ClientRequestContext ctx) {
final RpcRequest rpcReq = ctx.rpcRequest();
return rpcReq != null ? rpcReq.method() : ctx.method().name();
}

public static String path(ClientRequestContext ctx) {
final HttpRequest request = ctx.request();
return request == null ? "" : request.path();
}

private RequestContextUtil() {}
}
51 changes: 0 additions & 51 deletions ...n/java/com/linecorp/armeria/internal/common/circuitbreaker/CircuitBreakerMappingUtil.java
View file
Open in desktop

This file was deleted.

Loading
Loading