Introduce Preprocessor
#6057
Open
Introduce Preprocessor
#6057
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jrhee17
commented
Jan 8, 2025
core/src/main/java/com/linecorp/armeria/client/websocket/WebSocketClientBuilder.java
Outdated
Show resolved
Hide resolved
…cketClientBuilder.java
ikhoon
reviewed
Jan 8, 2025
core/src/main/java/com/linecorp/armeria/client/websocket/WebSocketClientBuilder.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/client/ClientPreprocessors.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/client/ClientPreprocessorsBuilder.java
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/client/HttpPreprocessor.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/client/DefaultWebClient.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/internal/client/DefaultClientRequestContext.java
Show resolved
Hide resolved
ikhoon
reviewed
Jan 9, 2025
| BiFunction<ClientRequestContext, Throwable, O> errorResponseFactory) { | ||
| final ClientRequestContextExtension ctxExt = ctx.as(ClientRequestContextExtension.class); | ||
| if (ctxExt != null) { | ||
| ctxExt.runContextCustomizer(); |
core/src/main/java/com/linecorp/armeria/client/RpcPreprocessor.java
Outdated
Show resolved
Hide resolved
grpc/src/main/java/com/linecorp/armeria/internal/client/grpc/ArmeriaClientCall.java
Outdated
Show resolved
Hide resolved
...thrift0.13/src/main/java/com/linecorp/armeria/internal/client/thrift/DefaultTHttpClient.java
Outdated
Show resolved
Hide resolved
|
@minwoox Do you have time to review this? |
minwoox
reviewed
Jan 13, 2025
| /** | ||
| * Sets the {@link EndpointGroup} used for the current {@link Request}. | ||
| */ | ||
| void endpointGroup(EndpointGroup endpointGroup); |
There was a problem hiding this comment.
setEndpointGroup?
We usually name the method in ClientRequestContext as setX when a getter of the X exists.
| @@ -83,7 +74,6 @@ protected UserClient(ClientBuilderParams params, Client<I, O> delegate, MeterReg | |||
| BiFunction<ClientRequestContext, Throwable, O> errorResponseFactory) { | |||
| super(delegate); | |||
| this.params = params; | |||
| this.meterRegistry = meterRegistry; | |||
There was a problem hiding this comment.
Could you remove this from the parameters?
Also, could you update HttpClientFactory.setMeterRegistry if you don't want to use this meter?
There was a problem hiding this comment.
I've just reverted to use HttpClientFactory.setMeterRegistry for backwards compatibility
| * The context customizer must be run before the following conditions. | ||
| * <li> | ||
| * <ul> | ||
| * EndpointSelector.select() so that the customizer can inject the attributes which may be |
There was a problem hiding this comment.
Please insert {@link ...}.
| @@ -113,12 +120,9 @@ public HttpResponse execute(HttpRequest req, RequestOptions requestOptions) { | |||
| newReq = req.withHeaders(req.headers().toBuilder().path(newPath)); | |||
| } | |||
|
|
|||
| return execute(protocol, | |||
There was a problem hiding this comment.
Would you add @Deprecated annotation to the execute methods?
minwoox
approved these changes
Jan 14, 2025
| * illustrates a sample use-case: | ||
| * <pre>{@code | ||
| * HttpPreprocessor preprocessor = (delegate, ctx, req) -> { | ||
| * ctx.endpointGroup(Endpoint.of("overriding-host")); |
There was a problem hiding this comment.
Suggested change
| * ctx.endpointGroup(Endpoint.of("overriding-host")); | |
| * ctx.setEndpointGroup(Endpoint.of("overriding-host")); |
| * illustrates a sample use-case: | ||
| * <pre>{@code | ||
| * RpcPreprocessor preprocessor = (delegate, ctx, req) -> { | ||
| * ctx.endpointGroup(Endpoint.of("overriding-host")); |
There was a problem hiding this comment.
Suggested change
| * ctx.endpointGroup(Endpoint.of("overriding-host")); | |
| * ctx.setEndpointGroup(Endpoint.of("overriding-host")); |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation:
This PR introduces the notion of
Preprocessors and allows users to configure these to clients as options.The second part of this PR will introduce a way for users to solely create a client based on
Preprocessors. The eventual POC can be found here: https://github.com/jrhee17/armeria/pull/36/filesEventually this extension point will also make it easier/clearer for users to use xDS with existing Armeria APIs.
The full capability/limitations/design of
Preprocessorsare better described in the following PR: #6051Modifications:
PreprocessorandPreClientAPIsClientPreprocessorsandClientPreprocessorsBuilderto allow users to easily addPreprocessors to clients as optionsDefaultWebClient,DefaultTHttpClient, andArmeriaClientCallto usePreprocessorsEndpointGroup, theEndpointGroupis now specified when creating aClientRequestContextinstead of at initialization time.ClientUtilmethods to pass an additionalreqfield which signifies the original request for type-safety.Result:
Preprocessors when creating a client.