Add basic documentation for xDS integration#6724
Draft
Conversation
Contributor
📝 WalkthroughWalkthroughPromotes XdsResourceReader from IT to the public xds module (YAML/JSON → Protobuf), updates ~25 tests to use the new type, adds whenReady() readiness signals in RouteConfigSelector and XdsPreprocessor, adds xDS docs and sidebar, and updates xds dependencies. Changes
Sequence Diagram(s)sequenceDiagram
participant Tester as "Test / YAML"
participant Reader as "XdsResourceReader\n(parse YAML/JSON → Proto)"
participant Bootstrap as "XdsBootstrap"
participant Preprocessor as "XdsPreprocessor"
participant Selector as "RouteConfigSelector"
participant Client as "WebClient / Requester"
Tester->>Reader: call fromYaml(yaml) / fromJson(json)
Reader-->>Tester: Bootstrap / Resource protobuf
Tester->>Bootstrap: XdsBootstrap.of(bootstrap)
Tester->>Preprocessor: XdsHttpPreprocessor.ofListener(listener, xdsBootstrap)
Preprocessor->>Selector: subscribe / request listener snapshot
Selector-->>Preprocessor: onListenerSnapshot(ListenerSnapshot)
Selector->>Selector: complete whenReady() future
Preprocessor-->>Tester: whenReady() completed
Tester->>Client: send request via WebClient.of(preprocessor)
Client->>Preprocessor: request passes through preprocessors (routing/TLS/etc.)
Preprocessor->>Client: response returned
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 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 |
![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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
xds/src/main/java/com/linecorp/armeria/xds/client/endpoint/RouteConfigSelector.java (1)
48-59:⚠️ Potential issue | 🟠 MajorDon't leave
whenReady()pending on bootstrap failure or shutdown.
SnapshotWatcher.onUpdate(...)can be invoked withsnapshot == nulland a non-nullThrowable, but this branch just returns. That meansXdsPreprocessor.whenReady().join()can block forever when the initial xDS fetch fails before the first snapshot. The same future also stays pending if the preprocessor is closed before readiness.Suggested fix
`@Override` public void onUpdate(`@Nullable` ListenerSnapshot snapshot, `@Nullable` Throwable t) { + if (t != null) { + whenReady.completeExceptionally(t); + } if (snapshot == null) { return; } routeConfig = new RouteConfig(snapshot); whenReady.complete(null); refresh(); } +void close() { + whenReady.cancel(false); +} + CompletableFuture<Void> whenReady() { return UnmodifiableFuture.wrap(whenReady); }Also wire the close path from
xds/src/main/java/com/linecorp/armeria/xds/client/endpoint/XdsPreprocessor.java:`@Override` public void close() { routeConfigSelector.close(); listenerRoot.close(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@xds/src/main/java/com/linecorp/armeria/xds/client/endpoint/RouteConfigSelector.java` around lines 48 - 59, The onUpdate method in RouteConfigSelector currently returns when snapshot == null, leaving the CompletableFuture whenReady() pending on bootstrap failure or shutdown; modify onUpdate(…) to detect snapshot == null with non-null Throwable and complete whenReady exceptionally with that Throwable (or completeExceptionally with a dedicated exception) so callers won't block, and also add a close() method on RouteConfigSelector that completes/cancels whenReady (or completesExceptionally with a ClosedException) and ensure XdsPreprocessor.close() calls routeConfigSelector.close() (in addition to listenerRoot.close()) so the readiness future is resolved on shutdown.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@site-new/sidebars.ts`:
- Around line 79-95: Update the xDS category items in sidebars.ts: remove the
non-existent entries 'advanced/xds/error-handling' and 'advanced/xds/internals'
and add the existing pages 'advanced/xds/introduction' and
'advanced/xds/concepts' to the items array for the category with label 'xDS' so
the listed items match the repository documentation.
In `@site-new/src/content/docs/advanced/xds/extensions.mdx`:
- Around line 36-60: The example implementation of HttpFilterFactory is out of
sync with the actual interface: rename the name() method to filterName(), remove
the nonexistent typeUrls() method entirely, and change the create(...) signature
to accept only (HttpFilter httpFilter, Any config) (drop the
XdsResourceValidator parameter); update the create implementation to
parse/unpack the Any config using available utilities inside the method body and
return an XdsHttpFilter (or null) accordingly so the implementation matches the
HttpFilterFactory contract.
In `@site-new/src/content/docs/advanced/xds/metrics.mdx`:
- Around line 33-35: Update the xdsType examples in the metrics doc so they
match emitted metric tag values: replace the plural examples (`listeners`,
`routes`, `clusters`, `endpoints`, `secrets`) with their singular forms used in
metrics/tests (`listener`, `route`, `cluster`, `endpoint`, `secret`) while
keeping `ads` as-is for the ADS stream; edit the xdsType line in the metrics.mdx
content to use these singular values so examples align with actual metric keys.
In
`@xds/src/main/java/com/linecorp/armeria/xds/client/endpoint/XdsPreprocessor.java`:
- Around line 87-93: The public method XdsPreprocessor.whenReady() is missing
the `@UnstableApi` annotation; add the `@UnstableApi` annotation to the whenReady()
method declaration so the unstable API surface is explicitly marked (the method
currently returns routeConfigSelector.whenReady())—ensure you import the
UnstableApi annotation and annotate the whenReady() method in class
XdsPreprocessor.
In `@xds/src/main/java/com/linecorp/armeria/xds/XdsResourceReader.java`:
- Around line 51-52: The class Javadoc for XdsResourceReader incorrectly claims
"All well-known Envoy protobuf types (under the io.envoyproxy package) are
automatically registered" while the actual registry only scans the
io.envoyproxy.envoy.extensions package; update the Javadoc in XdsResourceReader
to accurately describe the limited scan scope (e.g., state that only types under
io.envoyproxy.envoy.extensions are registered) so callers don't expect broader
Any resolution, or alternatively broaden the type-scanning implementation to
include the full io.envoyproxy package if you prefer that behavior; locate the
Javadoc around the XdsResourceReader class comment and adjust the wording to
match the registry behavior (or implement the broader scan in the registry
initialization code referenced by XdsResourceReader).
---
Outside diff comments:
In
`@xds/src/main/java/com/linecorp/armeria/xds/client/endpoint/RouteConfigSelector.java`:
- Around line 48-59: The onUpdate method in RouteConfigSelector currently
returns when snapshot == null, leaving the CompletableFuture whenReady() pending
on bootstrap failure or shutdown; modify onUpdate(…) to detect snapshot == null
with non-null Throwable and complete whenReady exceptionally with that Throwable
(or completeExceptionally with a dedicated exception) so callers won't block,
and also add a close() method on RouteConfigSelector that completes/cancels
whenReady (or completesExceptionally with a ClosedException) and ensure
XdsPreprocessor.close() calls routeConfigSelector.close() (in addition to
listenerRoot.close()) so the readiness future is resolved on shutdown.
🪄 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: 6ab1e7ce-6973-4720-a6fe-4ef4d46c62e3
📒 Files selected for processing (38)
it/xds-client/src/main/java/com/linecorp/armeria/xds/it/XdsResourceReader.javait/xds-client/src/test/java/com/linecorp/armeria/xds/client/endpoint/TransportSocketMatchUtilTest.javait/xds-client/src/test/java/com/linecorp/armeria/xds/it/BootstrapSecretsTest.javait/xds-client/src/test/java/com/linecorp/armeria/xds/it/BootstrapTest.javait/xds-client/src/test/java/com/linecorp/armeria/xds/it/CertificateValidationContextTest.javait/xds-client/src/test/java/com/linecorp/armeria/xds/it/ConfigSourceLifecycleObserverTest.javait/xds-client/src/test/java/com/linecorp/armeria/xds/it/ControlPlaneTlsIntegrationTest.javait/xds-client/src/test/java/com/linecorp/armeria/xds/it/DataSourceTest.javait/xds-client/src/test/java/com/linecorp/armeria/xds/it/DynamicSecretTest.javait/xds-client/src/test/java/com/linecorp/armeria/xds/it/ErrorHandlingTest.javait/xds-client/src/test/java/com/linecorp/armeria/xds/it/HealthCheckedTest.javait/xds-client/src/test/java/com/linecorp/armeria/xds/it/LoadBalancerReloadTest.javait/xds-client/src/test/java/com/linecorp/armeria/xds/it/PreprocessorErrorTest.javait/xds-client/src/test/java/com/linecorp/armeria/xds/it/ResourceNodeMetricTest.javait/xds-client/src/test/java/com/linecorp/armeria/xds/it/RetryTest.javait/xds-client/src/test/java/com/linecorp/armeria/xds/it/RouteMatcherTest.javait/xds-client/src/test/java/com/linecorp/armeria/xds/it/TimeoutTest.javait/xds-client/src/test/java/com/linecorp/armeria/xds/it/TlsPeerVerificationIntegrationTest.javait/xds-client/src/test/java/com/linecorp/armeria/xds/it/TlsValidationContextSdsIntegrationTest.javait/xds-client/src/test/java/com/linecorp/armeria/xds/it/TransportSocketMatchesIntegrationTest.javait/xds-client/src/test/java/com/linecorp/armeria/xds/it/TransportSocketSnapshotTest.javait/xds-client/src/test/java/com/linecorp/armeria/xds/it/VirtualHostRoutingTest.javait/xds-client/src/test/java/com/linecorp/armeria/xds/it/XdsEndpointGroupTest.javait/xds-client/src/test/java/com/linecorp/armeria/xds/it/XdsLoadBalancerLifecycleObserverTest.javait/xds-client/src/test/java/com/linecorp/armeria/xds/it/XdsPreprocessorTest.javasite-new/sidebars.tssite-new/src/content/docs/advanced/xds/comparison.mdxsite-new/src/content/docs/advanced/xds/concepts.mdxsite-new/src/content/docs/advanced/xds/extensions.mdxsite-new/src/content/docs/advanced/xds/getting-started.mdxsite-new/src/content/docs/advanced/xds/introduction.mdxsite-new/src/content/docs/advanced/xds/metrics.mdxsite-new/src/content/docs/advanced/xds/supported-features.mdxsite-new/src/content/docs/client/service-discovery.mdxxds/build.gradlexds/src/main/java/com/linecorp/armeria/xds/XdsResourceReader.javaxds/src/main/java/com/linecorp/armeria/xds/client/endpoint/RouteConfigSelector.javaxds/src/main/java/com/linecorp/armeria/xds/client/endpoint/XdsPreprocessor.java
💤 Files with no reviewable changes (1)
- it/xds-client/src/main/java/com/linecorp/armeria/xds/it/XdsResourceReader.java
Comment on lines
+36
to
+60
| ```java | ||
| import com.linecorp.armeria.xds.filter.HttpFilterFactory; | ||
| import com.linecorp.armeria.xds.filter.XdsHttpFilter; | ||
|
|
||
| public class MyFilterFactory implements HttpFilterFactory { | ||
|
|
||
| @Override | ||
| public String name() { | ||
| return "my.custom.filter"; | ||
| } | ||
|
|
||
| @Override | ||
| public List<String> typeUrls() { | ||
| return List.of("type.googleapis.com/my.custom.filter.v1.Config"); | ||
| } | ||
|
|
||
| @Override | ||
| @Nullable | ||
| public XdsHttpFilter create(HttpFilter httpFilter, Any config, | ||
| XdsResourceValidator validator) { | ||
| // Parse config and return an XdsHttpFilter, or null to skip | ||
| MyConfig myConfig = validator.unpack(config, MyConfig.class); | ||
| return new MyFilter(myConfig); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the HttpFilterFactory interface methods
ast-grep --pattern $'interface HttpFilterFactory {
$$$
}'Repository: line/armeria
Length of output: 38
🏁 Script executed:
#!/bin/bash
# Search for HttpFilterFactory interface definition in Java files
rg "interface HttpFilterFactory" -t java -A 20Repository: line/armeria
Length of output: 2609
🏁 Script executed:
#!/bin/bash
# Find Java files that might contain HttpFilterFactory
fd -e java | xargs grep -l "HttpFilterFactory" | head -20Repository: line/armeria
Length of output: 421
🏁 Script executed:
#!/bin/bash
# Search for the class definition more broadly
rg "class MyFilterFactory|interface HttpFilterFactory" -t java -B 2 -A 10Repository: line/armeria
Length of output: 1376
🏁 Script executed:
#!/bin/bash
# Read the complete HttpFilterFactory interface
cat xds/src/main/java/com/linecorp/armeria/xds/filter/HttpFilterFactory.javaRepository: line/armeria
Length of output: 2663
Update the HttpFilterFactory example to match the actual interface signature.
The code example contains three errors that will cause compilation failures:
- Method is
filterName(), notname() create()method takes only 2 parameters:(HttpFilter httpFilter, Any config), not 3 with aXdsResourceValidatorparameter- The
typeUrls()method doesn't exist in the interface and shouldn't be implemented
Corrected example
public class MyFilterFactory implements HttpFilterFactory {
`@Override`
- public String name() {
+ public String filterName() {
return "my.custom.filter";
}
- `@Override`
- public List<String> typeUrls() {
- return List.of("type.googleapis.com/my.custom.filter.v1.Config");
- }
-
`@Override`
`@Nullable`
- public XdsHttpFilter create(HttpFilter httpFilter, Any config,
- XdsResourceValidator validator) {
+ public XdsHttpFilter create(HttpFilter httpFilter, Any config) {
// Parse config and return an XdsHttpFilter, or null to skip
- MyConfig myConfig = validator.unpack(config, MyConfig.class);
+ MyConfig myConfig = config.unpack(MyConfig.class);
return new MyFilter(myConfig);
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@site-new/src/content/docs/advanced/xds/extensions.mdx` around lines 36 - 60,
The example implementation of HttpFilterFactory is out of sync with the actual
interface: rename the name() method to filterName(), remove the nonexistent
typeUrls() method entirely, and change the create(...) signature to accept only
(HttpFilter httpFilter, Any config) (drop the XdsResourceValidator parameter);
update the create implementation to parse/unpack the Any config using available
utilities inside the method body and return an XdsHttpFilter (or null)
accordingly so the implementation matches the HttpFilterFactory contract.
Comment on lines
+33
to
+35
| - `type` — the config source type (e.g. `ads`, `api_config_source`) | ||
| - `name` — the gRPC cluster name | ||
| - `xdsType` — the resource type (e.g. `listeners`, `routes`, `clusters`, `endpoints`, `secrets`) |
Contributor
There was a problem hiding this comment.
Align xdsType examples with emitted metric tag values.
Line 35 uses plural examples (listeners, routes, ...), but metric keys in tests use singular values like listener, cluster, route, endpoint (and ads for ADS stream), which can mislead readers.
💡 Suggested diff
-- `xdsType` — the resource type (e.g. `listeners`, `routes`, `clusters`, `endpoints`, `secrets`)
+- `xdsType` — the stream/resource type (e.g. `ads`, `listener`, `route`, `cluster`, `endpoint`, `secret`)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@site-new/src/content/docs/advanced/xds/metrics.mdx` around lines 33 - 35,
Update the xdsType examples in the metrics doc so they match emitted metric tag
values: replace the plural examples (`listeners`, `routes`, `clusters`,
`endpoints`, `secrets`) with their singular forms used in metrics/tests
(`listener`, `route`, `cluster`, `endpoint`, `secret`) while keeping `ads` as-is
for the ADS stream; edit the xdsType line in the metrics.mdx content to use
these singular values so examples align with actual metric keys.
Comment on lines
+87
to
+93
| /** | ||
| * Returns a {@link CompletableFuture} that completes when the initial xDS configuration | ||
| * has been received and the preprocessor is ready to route requests. | ||
| */ | ||
| public CompletableFuture<Void> whenReady() { | ||
| return routeConfigSelector.whenReady(); | ||
| } |
Contributor
There was a problem hiding this comment.
Mark whenReady() as unstable API.
This method becomes part of the public surface via the public xDS preprocessors, but it is missing @UnstableApi.
Suggested fix
+import com.linecorp.armeria.common.annotation.UnstableApi;
+
/**
* Returns a {`@link` CompletableFuture} that completes when the initial xDS configuration
* has been received and the preprocessor is ready to route requests.
*/
+ `@UnstableApi`
public CompletableFuture<Void> whenReady() {
return routeConfigSelector.whenReady();
}As per coding guidelines, "Review all newly added public classes and methods to ensure they have the @UnstableApi annotation."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@xds/src/main/java/com/linecorp/armeria/xds/client/endpoint/XdsPreprocessor.java`
around lines 87 - 93, The public method XdsPreprocessor.whenReady() is missing
the `@UnstableApi` annotation; add the `@UnstableApi` annotation to the whenReady()
method declaration so the unstable API surface is explicitly marked (the method
currently returns routeConfigSelector.whenReady())—ensure you import the
UnstableApi annotation and annotate the whenReady() method in class
XdsPreprocessor.
Comment on lines
+51
to
+52
| * <p>All well-known Envoy protobuf types (under the {@code io.envoyproxy} package) are | ||
| * automatically registered so that {@code @type} fields in YAML/JSON are resolved correctly. |
Contributor
There was a problem hiding this comment.
Javadoc overstates the registered type coverage.
The class comment says all well-known Envoy types under io.envoyproxy are registered, but the registry only scans io.envoyproxy.envoy.extensions. Please either widen the scan or narrow the Javadoc so callers do not expect broader Any resolution than this implementation provides.
Suggested doc-only fix
- * <p>All well-known Envoy protobuf types (under the {`@code` io.envoyproxy} package) are
- * automatically registered so that {`@code` `@type`} fields in YAML/JSON are resolved correctly.
+ * <p>Envoy extension protobuf types under {`@code` io.envoyproxy.envoy.extensions} are
+ * automatically registered so that {`@code` `@type`} fields in YAML/JSON are resolved correctly.Also applies to: 72-79
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@xds/src/main/java/com/linecorp/armeria/xds/XdsResourceReader.java` around
lines 51 - 52, The class Javadoc for XdsResourceReader incorrectly claims "All
well-known Envoy protobuf types (under the io.envoyproxy package) are
automatically registered" while the actual registry only scans the
io.envoyproxy.envoy.extensions package; update the Javadoc in XdsResourceReader
to accurately describe the limited scan scope (e.g., state that only types under
io.envoyproxy.envoy.extensions are registered) so callers don't expect broader
Any resolution, or alternatively broaden the type-scanning implementation to
include the full io.envoyproxy package if you prefer that behavior; locate the
Javadoc around the XdsResourceReader class comment and adjust the wording to
match the registry behavior (or implement the broader scan in the registry
initialization code referenced by XdsResourceReader).
Contributor
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@site-new/src/content/docs/advanced/xds/getting-started.mdx`:
- Around line 189-196: The cleanup snippet omits closing the RPC preprocessor
introduced earlier; update the cleanup block to also close rpcPreprocessor (in
addition to preprocessor and xdsBootstrap) or explicitly state this snippet is
HTTP-only; specifically locate the variables rpcPreprocessor, preprocessor, and
xdsBootstrap and either add a call to rpcPreprocessor.close() alongside
preprocessor.close() and xdsBootstrap.close(), or add a clarifying note above
the snippet that the example only demonstrates HTTP preprocessing and does not
manage rpcPreprocessor.
- Around line 121-127: The snippet declares xdsBootstrap twice causing a compile
error; split the mutually exclusive examples into separate code blocks and label
them (e.g., "Simple creation" using XdsBootstrap.of(bootstrap) and "Builder
customization" using
XdsBootstrap.builder(bootstrap).meterRegistry(...).build()), so each block shows
only one declaration and no duplicate variable assignments; update the
surrounding text so readers understand these are alternative approaches rather
than code to be copied together.
🪄 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: 88946eed-1734-4981-b9e5-cc9c820d6426
📒 Files selected for processing (6)
site-new/sidebars.tssite-new/src/content/docs/advanced/xds/extensions.mdxsite-new/src/content/docs/advanced/xds/getting-started.mdxsite-new/src/content/docs/advanced/xds/introduction.mdxsite-new/src/content/docs/advanced/xds/running-with-istio.mdxsite-new/src/content/docs/advanced/xds/supported-features.mdx
✅ Files skipped from review due to trivial changes (3)
- site-new/src/content/docs/advanced/xds/running-with-istio.mdx
- site-new/src/content/docs/advanced/xds/introduction.mdx
- site-new/src/content/docs/advanced/xds/extensions.mdx
🚧 Files skipped from review as they are similar to previous changes (1)
- site-new/sidebars.ts
Comment on lines
+121
to
+127
| // Simple creation | ||
| XdsBootstrap xdsBootstrap = XdsBootstrap.of(bootstrap); | ||
|
|
||
| // Or use the builder for customization | ||
| XdsBootstrap xdsBootstrap = XdsBootstrap.builder(bootstrap) | ||
| .meterRegistry(meterRegistry) | ||
| .build(); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd site-new && head -130 src/content/docs/advanced/xds/getting-started.mdx | tail -15Repository: line/armeria
Length of output: 545
Split mutually exclusive XdsBootstrap creation examples into separate code blocks.
In this Java snippet, xdsBootstrap is declared twice, which won't compile if copied as-is.
Suggested doc fix
-// Simple creation
-XdsBootstrap xdsBootstrap = XdsBootstrap.of(bootstrap);
-
-// Or use the builder for customization
-XdsBootstrap xdsBootstrap = XdsBootstrap.builder(bootstrap)
- .meterRegistry(meterRegistry)
- .build();
+// Option 1: simple creation
+XdsBootstrap xdsBootstrap = XdsBootstrap.of(bootstrap);// Option 2: builder customization
XdsBootstrap xdsBootstrap = XdsBootstrap.builder(bootstrap)
.meterRegistry(meterRegistry)
.build();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@site-new/src/content/docs/advanced/xds/getting-started.mdx` around lines 121
- 127, The snippet declares xdsBootstrap twice causing a compile error; split
the mutually exclusive examples into separate code blocks and label them (e.g.,
"Simple creation" using XdsBootstrap.of(bootstrap) and "Builder customization"
using XdsBootstrap.builder(bootstrap).meterRegistry(...).build()), so each block
shows only one declaration and no duplicate variable assignments; update the
surrounding text so readers understand these are alternative approaches rather
than code to be copied together.
Comment on lines
+189
to
+196
| ### Cleanup | ||
|
|
||
| Close the preprocessor and bootstrap when they are no longer needed: | ||
|
|
||
| ```java | ||
| preprocessor.close(); | ||
| xdsBootstrap.close(); | ||
| ``` |
Contributor
There was a problem hiding this comment.
Cleanup section should include the RPC preprocessor path shown earlier.
Line 146 introduces rpcPreprocessor, but cleanup only closes preprocessor. Please either add RPC cleanup or clarify this snippet is HTTP-only.
Suggested doc fix
preprocessor.close();
+rpcPreprocessor.close(); // if using Thrift/RPC path
xdsBootstrap.close();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@site-new/src/content/docs/advanced/xds/getting-started.mdx` around lines 189
- 196, The cleanup snippet omits closing the RPC preprocessor introduced
earlier; update the cleanup block to also close rpcPreprocessor (in addition to
preprocessor and xdsBootstrap) or explicitly state this snippet is HTTP-only;
specifically locate the variables rpcPreprocessor, preprocessor, and
xdsBootstrap and either add a call to rpcPreprocessor.close() alongside
preprocessor.close() and xdsBootstrap.close(), or add a clarifying note above
the snippet that the example only demonstrates HTTP preprocessing and does not
manage rpcPreprocessor.
ikhoon
approved these changes
Apr 15, 2026
|
|
||
| // Load the bootstrap injected by Istio | ||
| Bootstrap bootstrap = XdsResourceReader.fromJson( | ||
| Files.readString(Path.of("/etc/istio/proxy/envoy-rev.json")), |
Contributor
There was a problem hiding this comment.
Optional) Would it be convenient if we provide a factory method for this?
| * Returns a {@link CompletableFuture} that completes when the initial xDS configuration | ||
| * has been received and the preprocessor is ready to route requests. | ||
| */ | ||
| public CompletableFuture<Void> whenReady() { |
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
site-new/src/content/docs/advanced/xds/getting-started.mdx (2)
121-127:⚠️ Potential issue | 🟡 MinorSplit alternative
XdsBootstrapcreation into separate snippets.Line 122 and Line 125 both declare
xdsBootstrapin the same code block, so the snippet won’t compile if copied as-is. Keep these as mutually exclusive options.Suggested doc fix
-```java -import com.linecorp.armeria.xds.XdsBootstrap; - -// Simple creation -XdsBootstrap xdsBootstrap = XdsBootstrap.of(bootstrap); - -// Or use the builder for customization -XdsBootstrap xdsBootstrap = XdsBootstrap.builder(bootstrap) - .meterRegistry(meterRegistry) - .build(); -``` +```java +import com.linecorp.armeria.xds.XdsBootstrap; + +// Option 1: simple creation +XdsBootstrap xdsBootstrap = XdsBootstrap.of(bootstrap); +``` + +```java +import com.linecorp.armeria.xds.XdsBootstrap; + +// Option 2: builder customization +XdsBootstrap xdsBootstrap = XdsBootstrap.builder(bootstrap) + .meterRegistry(meterRegistry) + .build(); +```🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@site-new/src/content/docs/advanced/xds/getting-started.mdx` around lines 121 - 127, The two alternative XdsBootstrap creation examples should be split into separate, non-conflicting snippets so they compile when copied; replace the single code block that declares xdsBootstrap twice with two distinct examples: one showing XdsBootstrap.of(bootstrap) (simple creation) and a second showing XdsBootstrap.builder(bootstrap).meterRegistry(meterRegistry).build() (builder customization), ensuring each snippet imports com.linecorp.armeria.xds.XdsBootstrap and does not redeclare the same variable in the same block.
146-147:⚠️ Potential issue | 🟡 MinorCleanup example should also close
rpcPreprocessor(or be explicitly HTTP-only).Line 146 introduces
rpcPreprocessor, but the cleanup snippet on Lines 193-196 closes onlypreprocessorandxdsBootstrap.Suggested doc fix
```java preprocessor.close(); +rpcPreprocessor.close(); // if using the Thrift/RPC path xdsBootstrap.close();</details> Also applies to: 193-196 <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@site-new/src/content/docs/advanced/xds/getting-started.mdxaround lines 146
- 147, The cleanup example fails to close the RPC preprocessor introduced
earlier; when you create XdsRpcPreprocessor via
XdsRpcPreprocessor.ofListener("my-listener", xdsBootstrap) you must also close
that instance in the teardown. Update the cleanup snippet that currently calls
preprocessor.close() and xdsBootstrap.close() to also call
rpcPreprocessor.close() (or document that the example is HTTP-only and remove
rpcPreprocessor creation); reference XdsRpcPreprocessor/ofListener,
rpcPreprocessor, preprocessor, and xdsBootstrap to locate the spots to change.</details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In@site-new/src/content/docs/advanced/xds/getting-started.mdx:
- Around line 121-127: The two alternative XdsBootstrap creation examples should
be split into separate, non-conflicting snippets so they compile when copied;
replace the single code block that declares xdsBootstrap twice with two distinct
examples: one showing XdsBootstrap.of(bootstrap) (simple creation) and a second
showing XdsBootstrap.builder(bootstrap).meterRegistry(meterRegistry).build()
(builder customization), ensuring each snippet imports
com.linecorp.armeria.xds.XdsBootstrap and does not redeclare the same variable
in the same block.- Around line 146-147: The cleanup example fails to close the RPC preprocessor
introduced earlier; when you create XdsRpcPreprocessor via
XdsRpcPreprocessor.ofListener("my-listener", xdsBootstrap) you must also close
that instance in the teardown. Update the cleanup snippet that currently calls
preprocessor.close() and xdsBootstrap.close() to also call
rpcPreprocessor.close() (or document that the example is HTTP-only and remove
rpcPreprocessor creation); reference XdsRpcPreprocessor/ofListener,
rpcPreprocessor, preprocessor, and xdsBootstrap to locate the spots to change.</details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: Path: .coderabbit.yaml **Review profile**: CHILL **Plan**: Pro **Run ID**: `912c2bc7-145f-4367-8525-62aab6d836c0` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between aa1b278813cc53904bb8a95f3393c63df69595c6 and 9276a1e1f03534c7600fe9302e7b501dd1ebcc43. </details> <details> <summary>📒 Files selected for processing (6)</summary> * `it/xds-no-validation/src/test/java/com/linecorp/armeria/xds/it/RegressionGuard1Test.java` * `it/xds-no-validation/src/test/java/com/linecorp/armeria/xds/it/RegressionGuard2Test.java` * `site-new/src/content/docs/advanced/xds/concepts.mdx` * `site-new/src/content/docs/advanced/xds/getting-started.mdx` * `site-new/src/content/docs/advanced/xds/running-with-istio.mdx` * `xds/src/main/java/com/linecorp/armeria/xds/XdsResourceReader.java` </details> <details> <summary>✅ Files skipped from review due to trivial changes (3)</summary> * it/xds-no-validation/src/test/java/com/linecorp/armeria/xds/it/RegressionGuard1Test.java * site-new/src/content/docs/advanced/xds/running-with-istio.mdx * site-new/src/content/docs/advanced/xds/concepts.mdx </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
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
2 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:
Explain why you're making this change and what problem you're trying to solve.
Modifications:
Result: