Skip to content
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

Jump to bottom

Add documentation for preprocessors #6051

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

Add documentation for preprocessors #6051

wants to merge 2 commits into from
+246 −5

Conversation

jrhee17
Copy link
Contributor

@jrhee17 jrhee17 commented Dec 31, 2024 •
edited
Loading

Notes

  • This PR serves as a design proposal for the new preprocessors API and will be merged once features regarding preprocessors are fully implemented.
  • See Poc/xds filter v2 jrhee17/armeria#36 for details on proposed changes

localhost_8000_docs_advanced-client-preprocessor

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Dec 31, 2024 •
edited
Loading

🔍 Build Scan® (commit: 0f48096)

Job name Status Build Scan®
build-ubicloud-standard-8-jdk-8 ✅ https://ge.armeria.dev/s/4vawotrmypky6
build-ubicloud-standard-8-jdk-21-snapshot-blockhound ✅ https://ge.armeria.dev/s/76iefvbt3nazc
build-ubicloud-standard-8-jdk-17-min-java-17-coverage ✅ https://ge.armeria.dev/s/ohl6n5tmdnqi2
build-ubicloud-standard-8-jdk-17-min-java-11 ✅ https://ge.armeria.dev/s/yffonhfasko6y
build-ubicloud-standard-8-jdk-17-leak ✅ https://ge.armeria.dev/s/rcd7mvqxua7ai
build-ubicloud-standard-8-jdk-11 ✅ https://ge.armeria.dev/s/4ypuwyxrtbfte
build-macos-latest-jdk-21 ✅ https://ge.armeria.dev/s/cfrudzdrelc2w

minwoox
minwoox approved these changes Jan 2, 2025
View reviewed changes
Copy link
Contributor

@minwoox minwoox left a comment

Choose a reason for hiding this comment

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

The new interface looks promising. 👍 👍 👍

@minwoox
Copy link
Contributor

minwoox commented Jan 2, 2025

I also understand that the decorator chain is mutable so that different decorators can be added in the preprocessor.
e.g. Different Retrying client based on the RouteConfiguration.

ikhoon
ikhoon approved these changes Jan 7, 2025
View reviewed changes
Copy link
Contributor

@ikhoon ikhoon left a comment

Choose a reason for hiding this comment

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

👍👍

site/src/pages/docs/advanced-client-preprocessor.mdx

## Implementing `HttpPreprocessor` and `RpcPreprocessor`

<type://HttpPreprocessor> and <type://RpcPreprocessor> expose a <type://PartialClientRequestContext>.
Copy link
Contributor

Choose a reason for hiding this comment

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

I can understand what Partial means, but it might be ambiguous for normal users.
What do you think of prefixing Pre to imply it is used in *Preprocessor?

Suggested change
<type://HttpPreprocessor> and <type://RpcPreprocessor> expose a <type://PartialClientRequestContext>.
<type://HttpPreprocessor> and <type://RpcPreprocessor> expose a <type://PreClientRequestContext>.

site/src/pages/docs/advanced-client-preprocessor.mdx
+-----------+ +-----------------+ +-----------------+ +-----------------+ +----------+ +-----------+
```

Note that unlike decorators, <typeplural://HttpPreprocessor> are not executed for RPC-based clients.
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@jrhee17 jrhee17 mentioned this pull request Jan 8, 2025
Merged
jrhee17 added a commit that referenced this pull request Jan 15, 2025
@jrhee17
Introduce Preprocessor (#6057)
d72c4ce 
Motivation:

This PR introduces the notion of `Preprocessor`s 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 `Preprocessor`s. The eventual POC can be found
here: https://github.com/jrhee17/armeria/pull/36/files

Eventually this extension point will also make it easier/clearer for
users to use xDS with existing Armeria APIs.

The full capability/limitations/design of `Preprocessors` are better
described in the following PR: #6051

Modifications:

- Introduced `Preprocessor` and `PreClient` APIs
- Added `ClientPreprocessors` and `ClientPreprocessorsBuilder` to allow
users to easily add `Preprocessor`s to clients as options
- Modified `DefaultWebClient`, `DefaultTHttpClient`, and
`ArmeriaClientCall` to use `Preprocessor`s
- In order to allow users a way to overwrite the chosen `EndpointGroup`,
the `EndpointGroup` is now specified when creating a
`ClientRequestContext` instead of at initialization time.
- Modified `ClientUtil` methods to pass an additional `req` field which
signifies the original request for type-safety.

Result:

- Users can specify `Preprocessor`s when creating a client.

<!--
Visit this URL to learn more about how to write a pull request
description:

https://armeria.dev/community/developer-guide#how-to-write-pull-request-description
-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@minwoox minwoox minwoox approved these changes

@ikhoon ikhoon ikhoon approved these changes

@trustin trustin Awaiting requested review from trustin trustin is a code owner

Assignees
No one assigned
Labels
documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jrhee17 @minwoox @ikhoon