xDS retry now correctly re-selects endpoints on each retry attempt. - #6798
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughRefactors xDS routing and preprocessing: adds ClusterFilterFactory for per-cluster preprocessing and TLS injection, exposes cluster preprocessors, pre-builds route client chains with optional retry decoration, migrates SELECTED_ROUTE to XdsCommonUtil, and updates tests to expect EmptyEndpointGroupException. ChangesXDS Request Preprocessing Pipeline Refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
jrhee17
changed the title
jrhee17
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@xds/src/main/java/com/linecorp/armeria/xds/client/endpoint/XdsEndpointGroup.java`:
- Around line 90-92: The public factory method
XdsEndpointGroup.of(XdsLoadBalancer loadBalancer) must validate its parameter:
add an explicit Objects.requireNonNull(loadBalancer, "loadBalancer") at the
start of the of(...) method to fail fast with a clear message rather than
allowing later NPEs in the XdsEndpointGroup constructor; update imports if
needed and leave the constructor unchanged (refer to XdsEndpointGroup.of and the
XdsLoadBalancer parameter).
In `@xds/src/main/java/com/linecorp/armeria/xds/RouteEntry.java`:
- Around line 147-148: Add an explicit null-check at the start of the public
method matches(ClientRequestContext ctx) in class RouteEntry by calling
Objects.requireNonNull(ctx, "ctx") before using matcher.matches(ctx); also
ensure java.util.Objects is imported if not already so the method fails fast
with a clear NPE message rather than relying on matcher.matches to throw later.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e963e58a-98e0-4792-a92d-83dcacc99076
📒 Files selected for processing (16)
it/xds-client/src/test/java/com/linecorp/armeria/xds/it/PreprocessorErrorTest.javait/xds-client/src/test/java/com/linecorp/armeria/xds/it/RetryTest.javaxds/src/main/java/com/linecorp/armeria/xds/ClusterFilterFactory.javaxds/src/main/java/com/linecorp/armeria/xds/ClusterSnapshot.javaxds/src/main/java/com/linecorp/armeria/xds/FilterUtil.javaxds/src/main/java/com/linecorp/armeria/xds/GrpcServicesPreprocessor.javaxds/src/main/java/com/linecorp/armeria/xds/RouteEntry.javaxds/src/main/java/com/linecorp/armeria/xds/RouteStream.javaxds/src/main/java/com/linecorp/armeria/xds/client/endpoint/RouterFilter.javaxds/src/main/java/com/linecorp/armeria/xds/client/endpoint/RouterFilterFactory.javaxds/src/main/java/com/linecorp/armeria/xds/client/endpoint/XdsAttributeKeys.javaxds/src/main/java/com/linecorp/armeria/xds/client/endpoint/XdsEndpointGroup.javaxds/src/main/java/com/linecorp/armeria/xds/client/endpoint/XdsHttpPreprocessor.javaxds/src/main/java/com/linecorp/armeria/xds/client/endpoint/XdsRpcPreprocessor.javaxds/src/main/java/com/linecorp/armeria/xds/internal/XdsCommonUtil.javaxds/src/test/java/com/linecorp/armeria/xds/client/endpoint/RouteMetadataSubsetTest.java
💤 Files with no reviewable changes (1)
- xds/src/main/java/com/linecorp/armeria/xds/client/endpoint/XdsAttributeKeys.java
Contributor
jrhee17
marked this pull request as ready for review
June 8, 2026 06:02
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6798 +/- ##
============================================
- Coverage 74.46% 0 -74.47%
============================================
Files 1963 0 -1963
Lines 82437 0 -82437
Branches 10764 0 -10764
============================================
- Hits 61385 0 -61385
+ Misses 15918 0 -15918
+ Partials 5134 0 -5134 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
minwoox
approved these changes
Jun 9, 2026
|
|
||
| final class RouterFilter<I extends Request, O extends Response> implements Preprocessor<I, O> { | ||
|
|
||
| private static final AttributeKey<RouteEntry> SELECTED_ROUTE = XdsCommonUtil.SELECTED_ROUTE; |
Contributor
There was a problem hiding this comment.
Nit: I think we can just inline it.
| private <I extends Request, O extends Response> O execute( | ||
| PreClient<I, O> delegate, PreClientRequestContext ctx, I req) throws Exception { | ||
| if (endpointGroup == null) { | ||
| throw UnprocessedRequestException.of(new IllegalStateException( |
Contributor
There was a problem hiding this comment.
Can't we throw the exception in the ctor?
Contributor
Author
There was a problem hiding this comment.
Modified s.t. we enforce either a load_assignment or eds_cluster_config is specified at 3bd9524
ikhoon
approved these changes
Jun 9, 2026
| XdsCommonUtil.setTlsParams(ctx, endpoint); | ||
| ctx.setEndpointGroup(endpoint); | ||
| return delegate.execute(ctx, req); | ||
| return preprocessorMapper.apply(clusterSnapshot).execute(delegate, ctx, req); |
Contributor
There was a problem hiding this comment.
Optional) If the preprocessor is just obtained from the clusterSnapshot, should we take a flag for simpilicy?
Preprocessor<...,...> preprocessor;
if (isRpc) {
preprocessor = clusterSnapshot.rpcPreprocessor();
} else {
preprocessor = clusterSnapshot.preprocessor();
}
Contributor
Author
There was a problem hiding this comment.
Done with an unsafe cast
| final ClusterFilterFactory factory = new ClusterFilterFactory( | ||
| clusterXdsResource, this.loadBalancer, transportSocket); | ||
| httpPreprocessor = factory.httpPreprocessor(); | ||
| rpcPreprocessor = factory.rpcPreprocessor(); |
Contributor
There was a problem hiding this comment.
Question) Don't we need to update toString(), hashCode() and equals()?
Nit) Since hashCode() and equals() are usually changed together, it would be better to place them together.
Contributor
Author
There was a problem hiding this comment.
httpPreprocessor, rpcPreprocessor are derived fields - I'm not sure if it's necessary to add these to equality/hashcode checks.
Added httpPreprocessor, rpcPreprocessor to debugString
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
xds/src/test/java/com/linecorp/armeria/xds/XdsTestUtil.java (1)
2-5:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the Java file header to the required LY Corporation form.
This touched Java file still uses
LINE Corporationin the copyright header.Suggested patch
- * Copyright 2025 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,As per coding guidelines, "For any Java changes in this PR: add/keep the required LY Corporation copyright header (don’t churn the year on every file)."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@xds/src/test/java/com/linecorp/armeria/xds/XdsTestUtil.java` around lines 2 - 5, The file header in XdsTestUtil.java still uses "LINE Corporation"; update the top-of-file Java header to the required "LY Corporation" form while preserving the existing year and license text (do not change the year). Locate the header comment above the class XdsTestUtil and replace occurrences of "LINE Corporation" with "LY Corporation" so the file complies with the project's copyright header guideline.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@xds/src/test/java/com/linecorp/armeria/xds/XdsTestUtil.java`:
- Around line 2-5: The file header in XdsTestUtil.java still uses "LINE
Corporation"; update the top-of-file Java header to the required "LY
Corporation" form while preserving the existing year and license text (do not
change the year). Locate the header comment above the class XdsTestUtil and
replace occurrences of "LINE Corporation" with "LY Corporation" so the file
complies with the project's copyright header guideline.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bfa520a6-9467-42e0-b50d-4e3684527efc
📒 Files selected for processing (5)
xds/src/main/java/com/linecorp/armeria/xds/ClusterFilterFactory.javaxds/src/main/java/com/linecorp/armeria/xds/ClusterSnapshot.javaxds/src/main/java/com/linecorp/armeria/xds/ClusterStream.javaxds/src/main/java/com/linecorp/armeria/xds/client/endpoint/XdsEndpointGroup.javaxds/src/test/java/com/linecorp/armeria/xds/XdsTestUtil.java
💤 Files with no reviewable changes (1)
- xds/src/main/java/com/linecorp/armeria/xds/client/endpoint/XdsEndpointGroup.java
🚧 Files skipped from review as they are similar to previous changes (1)
- xds/src/main/java/com/linecorp/armeria/xds/ClusterSnapshot.java
This was referenced Jul 2, 2026
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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:
xDS retry was broken because
RetryingClientwas built as part of the upstreamClientDecorationinsideFilterUtil.buildUpstreamFilter(). This placed the retry decorator after endpoint selection, so retries always reused the same endpoint instead of re-selecting. The fix separates retry from upstream filters and places it before the cluster decorator chain, so each retry attempt goes through endpoint selection again.Additionally, the cluster-level preprocessor, decorator, and retry logic were scattered across
ClusterFilter,RouteEntry, andRouterFilter, making the execution order hard to follow.Modifications:
FilterUtil.buildRetryDecoration()so retry wraps the full cluster chain inRouteEntry:[retry] -> [cluster decorator] -> [upstream decorators] -> [delegate]ClusterFilterFactorywhich consolidates cluster-level preprocessor and decorator logic into a single class with a clear execution order.ClusterSnapshot.hasPreprocessor()/executePreprocessor()with typed, non-nullpreprocessor()→HttpPreprocessorandrpcPreprocessor()→RpcPreprocessor.RouterFilterto accept aFunction<ClusterSnapshot, Preprocessor>mapping, wired viaClusterSnapshot::preprocessor/ClusterSnapshot::rpcPreprocessorinRouterFilterFactory.GrpcServicesPreprocessorto usesnapshot.preprocessor()instead of manual endpoint selection and TLS setup.SELECTED_ROUTEattribute key toXdsCommonUtilfor cross-package accessibility.ClusterFilter,XdsCommonUtil.setTlsParams(),ClusterSnapshot.endpointGroup(), andClusterSnapshot.sessionProtocol().Result:
ClusterFilterFactory:ClusterSnapshot.preprocessor()andClusterSnapshot.rpcPreprocessor()provide typed, non-null accessors enabling direct use likeWebClient.of(clusterSnapshot.preprocessor()).