Introduce `Preprocessor` based clients #6060

jrhee17 · 2025-01-09T03:00:00Z

Motivation:

#6057 has described preprocessors, which allow users to dynamically modify how to connect to a target endpoint dynamically. However, there is a limitation where an EndpointGroup and SessionProtocol must be provided even though the Preprocessor will solely determine how to connect to a remote.

For example, specifying the protocol and endpoint when building the client is not necessary since the preprocessor will fill in all of the necessary information.

HttpPreprocessor preprocessor = (delegate, ctx, req) -> {
  ctx.setEndpoint(Endpoint.of(...));
  ctx.setSessionProtocol(HTTPS);
  return delegate.execute(ctx, req);
}
GrpcClients.of(HTTP, Endpoint.of(...)).preprocessor(preprocessor)

I propose that users are able to build a client solely based on Preprocessor. By doing so, we may later introduce a XdsPreprocessor which may dynamically determine how to connect to a remote based on resources.

GrpcClients.of(XdsPreprocessor.of(...));

In order to support the above, I propose the following:

If a client is built based on Preprocessor, the SessionProtocol must be UNDEFINED.
- For instance, GrpcClients.of(HttpPreprocessor).params().scheme().sessionProtocol() will be UNDEFINED
If a client is built with SessionProtocol.UNDEFINED, at least one Preprocessor must be present in ClientOptions.
- For instance, a client built via (WebClient.of("undefined://google.com")) must also supply a preprocessor.
- An exception to this rule is WebClient.of(). Because it is easy to supply a target EndpointGroup, SessionProtocol from its API, a preprocessor is not necessarily for creating a default WebClient.

Modifications:

User facing APIs have been added which allow clients to be initialized solely based on a Preprocessor.
- Users may specify Preprocessor, and optionally a SerializationFormat and prefix path.

With the addition preprocessor-based clients, it is important to validate whether a client was created incorrectly. In order to validate client parameters from a unified location, it is necessary to know how a client was created. For this reason, following changes have been made to ClientBuilderParams:

If a client is not created based on a URI, create an internal URI based on a rule so that we can check how a client was created. e.g. endpointGroup based clients will have prefix armeria-group-, preprocessor based clients will have prefix armeria-preprocessor-, etc...
- ClientBuilderParamsUtil has utility methods which determines how a client has been created.
Introduce a ClientBuilderParamsBuilder which introduces a fluent way to create a new ClientBuilderParams based on an existing ClientBuilderParams. This will help preserve the URI authority which embeds how a client was created.

Result:

Users can create clients solely based on Preprocessor

Miscellaneous

Just because an exception has been thrown from preprocessing does not necessarily mean that the request has not been sent. I have removed the UnprocessedRequestException wrapping behavior.
e.g.

val preprocessor = (delegate, ctx, req) -> {
  val res = delegate.execute(ctx, req);
  throw new RuntimeException();
}

github-actions · 2025-01-09T04:13:03Z

🔍 Build Scan® (commit: `71a5785`)

Job name	Status	Build Scan®
build-ubicloud-standard-16-jdk-8	✅	https://ge.armeria.dev/s/mctz6odzzul2s
build-ubicloud-standard-16-jdk-21-snapshot-blockhound	✅	https://ge.armeria.dev/s/wpeuk3gyvbj7e
build-ubicloud-standard-16-jdk-17-min-java-17-coverage	✅	https://ge.armeria.dev/s/vmjypgk6rsvpo
build-ubicloud-standard-16-jdk-17-min-java-11	✅	https://ge.armeria.dev/s/3y7v45n3qbb5u
build-ubicloud-standard-16-jdk-17-leak	✅	https://ge.armeria.dev/s/gxop6oymvnc7k
build-ubicloud-standard-16-jdk-11	✅	https://ge.armeria.dev/s/y5wqn3njzazhm
build-macos-latest-jdk-21	✅	https://ge.armeria.dev/s/r7xpncjzolwms

jrhee17 · 2025-01-15T08:21:35Z

ready for review

ikhoon · 2025-01-20T10:39:40Z

core/src/main/java/com/linecorp/armeria/common/SessionProtocol.java

-    PROXY("proxy", false, false, 0);
+    PROXY("proxy", false, false, 0),
+
+    UNDEFINED("undefined", false, false, 0);


Question) What do you think of using HTTP as the default session protocol instead? If a scheme is missing in curl or a browser, HTTP protocol is used by default.

$ curl google.com -v * Host google.com:80 was resolved. ...

Within Armeria, HTTP is currently not the default session protocol.

e.g. the following API exists

WebClient of(SessionProtocol protocol, EndpointGroup endpointGroup)

but the following API doesn't

WebClient of(EndpointGroup endpointGroup)

I'm fine with setting HTTP as the default session protocol, but for consistency I prefer that all APIs reflect this default session protocol behavior.

WebClient of(EndpointGroup endpointGroup)

It would also be a good idea to add this API in the future.

SessionProtocol.UNDEFINED is unlikely to be used by users, and additional validation is required in places where UNDEFINED is not allowed.

Personally, I doubt that SessionProtocol.PROXY was a good decision. Because the usage of SessionProtocol.PROXY is limited only when specifying a port for creating a server. The rest of our codebase has a lot of validation added to check if it is HTTP or HTTPS.

It would also be a good idea to add this API in the future.

If the others agree with this direction, let me also add the relevant APIs in this PR then.

I also prefer not using SessionProtocol.UNDEFINED if we can avoid it. 😉

core/src/main/java/com/linecorp/armeria/client/ClientBuilderParams.java

ikhoon · 2025-01-20T10:49:38Z

core/src/main/java/com/linecorp/armeria/client/ClientBuilderParamsBuilder.java

+/**
+ * Allows creation of a new {@link ClientBuilderParams} based on a previous {@link ClientBuilderParams}.
+ */
+public final class ClientBuilderParamsBuilder {


Question) Could we use this builder to create DefaultClientBuilderParams in ClientBuilderParams.of()?

ikhoon · 2025-01-20T11:12:51Z

core/src/main/java/com/linecorp/armeria/internal/client/endpoint/FailingEndpointGroup.java

+import com.linecorp.armeria.common.annotation.Nullable;
+import com.linecorp.armeria.common.util.UnmodifiableFuture;
+
+public final class FailingEndpointGroup implements EndpointGroup {


Optional) How about exposing FailingEndpointGroup as a public API or its singleton instance? It may be useful for a user to check whether the current EndpointGroup is initialized in Preprocessor to conditionally create an EndpointGroup.

UninitializedEndpointGroup or UndefinedEndpointGroup?
How about raising the exception for all method calls?

ikhoon · 2025-01-20T11:29:33Z

core/src/main/java/com/linecorp/armeria/internal/client/ClientBuilderParamsUtil.java

+    private static final String PREPROCESSOR_PREFIX = "armeria-preprocessor-";
+    public static final String ENDPOINTGROUP_PREFIX = "armeria-group-";
+    public static final String UNDEFINED_URI_AUTHORITY = "armeria-undefined:1";


Question) What do you think about also using UNDEFINED_URI_AUTHORITY as a placeholder for Preprocessor to indicate that the endpoint needs initializing?

If SerializationFormat is needed, I wonder if it would also be possible to express it like this:

gproto+http://armeria-undefined:1

I'm not sure I understood the question. Are you suggesting that we treat UNDEFINED_URI_AUTHORITY and Preprocessor equivalently?

I was wondering if it was possible to reuse the existing undefined URI, UNDEFINED_URI_AUTHORITY rather than creating a new one.

minwoox · 2025-01-17T13:20:28Z

core/src/main/java/com/linecorp/armeria/client/ClientBuilderParams.java

+     * Returns a {@link ClientBuilderParamsBuilder} which allows creation of a new
+     * {@link ClientBuilderParams} based on the current properties.
+     */
+    default ClientBuilderParamsBuilder paramsBuilder() {


Global comment: @UnstableApi

minwoox · 2025-01-17T13:20:49Z

core/src/main/java/com/linecorp/armeria/client/ClientBuilderParamsBuilder.java

+     * Sets the {@link SerializationFormat} for this {@link ClientBuilderParams}.
+     */
+    public ClientBuilderParamsBuilder serializationFormat(SerializationFormat serializationFormat) {
+        this.serializationFormat = serializationFormat;


rnn for all methods?

minwoox · 2025-01-17T13:23:59Z

core/src/main/java/com/linecorp/armeria/client/Clients.java

+     *                                  the specified {@link SerializationFormat} or
+     *                                  {@link ClientPreprocessors}
+     */
+    public static <T> T  newClient(ClientPreprocessors preprocessors, Class<T> clientType) {


Suggested change

public static <T> T newClient(ClientPreprocessors preprocessors, Class<T> clientType) {

public static <T> T newClient(ClientPreprocessors preprocessors, Class<T> clientType) {

minwoox · 2025-01-17T13:24:38Z

core/src/main/java/com/linecorp/armeria/client/Clients.java

+     * @param clientType the type of the new client
+     *
+     * @throws IllegalArgumentException if the specified {@code clientType} is unsupported for
+     *                                  the specified {@link SerializationFormat} or


SerializationFormat is not specfied

core/src/main/java/com/linecorp/armeria/client/ClientBuilderParams.java

minwoox

Looks good all in all. 👍

minwoox · 2025-01-21T01:04:07Z

core/src/main/java/com/linecorp/armeria/client/Clients.java

-        return uri == UNDEFINED_URI;
+        return (uri == ClientUtil.UNDEFINED_URI ||
+                ClientBuilderParamsUtil.UNDEFINED_URI_AUTHORITY.equals(uri.getAuthority())) &&
+                uri.getPort() == 1;


We can probably remove uri.getPort() == 1 because the above conditions already imply it.

minwoox · 2025-01-21T01:15:26Z

core/src/main/java/com/linecorp/armeria/internal/client/endpoint/FailingEndpointGroup.java

+import com.linecorp.armeria.common.annotation.Nullable;
+import com.linecorp.armeria.common.util.UnmodifiableFuture;
+
+public final class FailingEndpointGroup implements EndpointGroup {


UninitializedEndpointGroup or UndefinedEndpointGroup?
How about raising the exception for all method calls?

minwoox · 2025-01-21T01:16:43Z

core/src/main/java/com/linecorp/armeria/client/DefaultClientBuilderParams.java

@@ -89,7 +94,7 @@ final class DefaultClientBuilderParams implements ClientBuilderParams {
                             normalizedAbsolutePathRef);
        } else {
            // Create a valid URI which will never succeed.
-            uri = URI.create(schemeStr + "://armeria-group-" +
+            uri = URI.create(schemeStr + "://" + ClientBuilderParamsUtil.ENDPOINTGROUP_PREFIX +


nit: ENDPOINT_GROUP_PREFIX

minwoox · 2025-01-21T01:18:36Z

core/src/main/java/com/linecorp/armeria/client/DefaultWebClient.java

-                            "Scheme and authority must be specified in \":path\" or " +
-                            "in \":scheme\" and \":authority\". :path=" +
-                            originalPath + ", :scheme=" + req.scheme() + ", :authority=" + req.authority()));
+                            "Failed to parse a scheme: " + reqTarget.scheme(), e));


Suggested change

"Failed to parse a scheme: " + reqTarget.scheme(), e));

"Failed to parse a scheme: " + scheme, e));

minwoox · 2025-01-21T10:23:27Z

core/src/main/java/com/linecorp/armeria/client/ClientBuilderParamsBuilder.java

+        if (scheme.serializationFormat() == SerializationFormat.NONE) {
+            schemeStr = scheme.sessionProtocol().uriText();
+        } else {
+            schemeStr = scheme.uriText();
+        }


Suggested change

if (scheme.serializationFormat() == SerializationFormat.NONE) {

schemeStr = scheme.sessionProtocol().uriText();

} else {

schemeStr = scheme.uriText();

}

if (scheme.serializationFormat() == SerializationFormat.NONE) {

schemeStr = scheme.sessionProtocol().uriText();

} else {

schemeStr = scheme.uriText();

}

I've seen this logic in multiple places. How about extracting it into a method? e.g. scheme.effectiveUriText()

minwoox · 2025-01-21T12:13:12Z

core/src/main/java/com/linecorp/armeria/common/SessionProtocol.java

-    PROXY("proxy", false, false, 0);
+    PROXY("proxy", false, false, 0),
+
+    UNDEFINED("undefined", false, false, 0);


I also prefer not using SessionProtocol.UNDEFINED if we can avoid it. 😉

minwoox · 2025-01-21T12:16:17Z

grpc/src/main/java/com/linecorp/armeria/client/grpc/GrpcClients.java

+    @UnstableApi
+    public static GrpcClientBuilder builder(HttpPreprocessor httpPreprocessor) {
+        requireNonNull(httpPreprocessor, "httpPreprocessor");
+        return builder(SerializationFormat.NONE, httpPreprocessor);


Shouldn't it be GrpcSerializationFormats.PROTO?

minwoox · 2025-01-21T12:18:49Z

thrift/thrift0.13/src/main/java/com/linecorp/armeria/client/thrift/ThriftClients.java

+     */
+    @UnstableApi
+    public static ThriftClientBuilder builder(RpcPreprocessor rpcPreprocessor) {
+        return new ThriftClientBuilder(SerializationFormat.NONE,


Isnt' this ThriftSerializationFormats.BINARY?

jrhee17 mentioned this pull request Jan 9, 2025

Introduce Preprocessor #6057

Merged

jrhee17 force-pushed the feat/xds-filter-v2 branch 2 times, most recently from 64371bc to e111262 Compare January 15, 2025 07:36

poc

23a2685

jrhee17 force-pushed the feat/xds-filter-v2 branch from e111262 to 23a2685 Compare January 15, 2025 07:38

jrhee17 marked this pull request as ready for review January 15, 2025 08:21

jrhee17 requested review from ikhoon, minwoox and trustin as code owners January 15, 2025 08:21

jrhee17 added this to the 1.32.0 milestone Jan 15, 2025

jrhee17 added the new feature label Jan 15, 2025

ikhoon reviewed Jan 20, 2025

View reviewed changes

minwoox reviewed Jan 21, 2025

View reviewed changes

address comments by @ikhoon and @minwoox

71a5785

minwoox reviewed Jan 21, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce `Preprocessor` based clients #6060

Introduce `Preprocessor` based clients #6060

jrhee17 commented Jan 9, 2025 •

edited

Loading

github-actions bot commented Jan 9, 2025 •

edited

Loading

jrhee17 commented Jan 15, 2025

ikhoon Jan 20, 2025

jrhee17 Jan 21, 2025

ikhoon Jan 21, 2025

jrhee17 Jan 21, 2025

minwoox Jan 21, 2025

ikhoon Jan 20, 2025

ikhoon Jan 20, 2025

minwoox Jan 21, 2025

ikhoon Jan 20, 2025

jrhee17 Jan 21, 2025

ikhoon Jan 22, 2025 •

edited

Loading

minwoox Jan 17, 2025

minwoox Jan 17, 2025

minwoox Jan 17, 2025

minwoox Jan 17, 2025

minwoox left a comment

minwoox Jan 21, 2025

minwoox Jan 21, 2025

minwoox Jan 21, 2025

minwoox Jan 21, 2025

minwoox Jan 21, 2025

minwoox Jan 21, 2025

minwoox Jan 21, 2025

minwoox Jan 21, 2025

	public static <T> T newClient(ClientPreprocessors preprocessors, Class<T> clientType) {
	public static <T> T newClient(ClientPreprocessors preprocessors, Class<T> clientType) {

	"Failed to parse a scheme: " + reqTarget.scheme(), e));
	"Failed to parse a scheme: " + scheme, e));

Introduce Preprocessor based clients #6060

Are you sure you want to change the base?

Introduce Preprocessor based clients #6060

Conversation

jrhee17 commented Jan 9, 2025 • edited Loading

github-actions bot commented Jan 9, 2025 • edited Loading

🔍 Build Scan® (commit: 71a5785)

jrhee17 commented Jan 15, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ikhoon Jan 22, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minwoox left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Introduce `Preprocessor` based clients #6060

Introduce `Preprocessor` based clients #6060

jrhee17 commented Jan 9, 2025 •

edited

Loading

github-actions bot commented Jan 9, 2025 •

edited

Loading

🔍 Build Scan® (commit: `71a5785`)

ikhoon Jan 22, 2025 •

edited

Loading