Draft
Conversation
Contributor
📝 WalkthroughWalkthroughAdds delta xDS support, an extension/validation registry, new stream implementations and state coordination, refactors filter/transport-socket factory wiring, changes resource parsing signatures to accept an extension registry, and expands integration/unit tests covering lifecycle, metrics, and error scenarios. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant AdsXdsStream
participant DeltaActualStream
participant ControlPlane as gRPC Control Plane
participant StateCoordinator
participant ResourceWatcher
Client->>AdsXdsStream: start()
AdsXdsStream->>DeltaActualStream: create/open stream
DeltaActualStream->>ControlPlane: open bidirectional stream
ControlPlane-->>DeltaActualStream: DeltaDiscoveryResponse
DeltaActualStream->>DeltaActualStream: parse resources
alt valid
DeltaActualStream->>StateCoordinator: onResourceUpdated(type,name,resource)
StateCoordinator->>ResourceWatcher: onChanged(snapshot)
else invalid
DeltaActualStream->>ControlPlane: send NACK with error_detail
end
DeltaActualStream->>ControlPlane: send ACK with nonce
sequenceDiagram
participant Client
participant ConfigSourceClient
participant StateCoordinator
participant SubscriberStorage
participant AdsXdsStream
Client->>ConfigSourceClient: subscribe(type,name,watcher)
ConfigSourceClient->>StateCoordinator: register(type,name,watcher)
StateCoordinator->>SubscriberStorage: register watcher
StateCoordinator->>StateCoordinator: replayToWatcher if cached
alt not cached
StateCoordinator->>AdsXdsStream: ensure stream / resourcesUpdated(type)
AdsXdsStream->>AdsXdsStream: enqueue/send discovery request
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
|
![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: 7
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/XdsStreamSubscriber.java (1)
56-65:⚠️ Potential issue | 🟡 Minor
restartTimer()can accumulate multiple pending absent timers.If this method is called more than once before timeout, prior scheduled tasks are not canceled, which can trigger duplicate
onAbsent()notifications.Suggested fix
void restartTimer() { if (!enableAbsentOnTimeout) { return; } + maybeCancelAbsentTimer(); initialAbsentFuture = eventLoop.schedule(() -> { initialAbsentFuture = null; onAbsent(); }, timeoutMillis, TimeUnit.MILLISECONDS); }🤖 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/XdsStreamSubscriber.java` around lines 56 - 65, restartTimer() can schedule multiple pending timers because it doesn't cancel any previously scheduled initialAbsentFuture; modify restartTimer() to check initialAbsentFuture and cancel it (e.g., initialAbsentFuture.cancel(false)) if it's non-null and not done before assigning a new scheduled future on eventLoop, preserving existing checks for enableAbsentOnTimeout and retaining the same behavior that clears initialAbsentFuture and calls onAbsent() when the timeout fires.
🧹 Nitpick comments (6)
xds/src/main/java/com/linecorp/armeria/xds/CompositeXdsStream.java (1)
28-38: Document stream supplier contract or add deduplication.The API accepts a supplier with no explicit guarantee that distinct stream instances are created per
XdsType. All current call sites correctly create distinct instances, but the contract should be clarified. Either document that suppliers must return distinct instances, or add identity deduplication inclose()to guard against accidental reuse.🤖 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/CompositeXdsStream.java` around lines 28 - 38, The constructor of CompositeXdsStream accepts a streamSupplier without guaranteeing distinct instances per XdsType; to fix, update the contract and add identity-based deduplication in close(): in CompositeXdsStream.close() use an identity-based Set (e.g., Collections.newSetFromMap(new IdentityHashMap<>())) to track seen XdsStream instances and call close() only once per unique instance; mention in the constructor/Javadoc that suppliers should preferably return distinct instances but the close() protects against accidental reuse of the same instance across types.xds/src/main/java/com/linecorp/armeria/xds/SecretXdsResource.java (1)
41-43: Add a defensive null-check forsecretin constructor.This keeps failure mode explicit at construction time rather than surfacing as later NPEs.
As per coding guidelines, follow LY OSS style for null checks using `Objects.requireNonNull`.Suggested fix
private SecretXdsResource(Secret secret, String version, long revision) { super(version, revision); - this.secret = secret; + this.secret = java.util.Objects.requireNonNull(secret, "secret"); }🤖 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/SecretXdsResource.java` around lines 41 - 43, Add a defensive null-check in the SecretXdsResource constructor: use Objects.requireNonNull on the secret parameter (in SecretXdsResource(Secret secret, String version, long revision)) and provide a clear message (e.g., "secret") so construction fails fast; also ensure java.util.Objects is imported if not already and then assign the checked value to the instance field 'secret'.xds/src/main/java/com/linecorp/armeria/xds/XdsUnpackUtil.java (1)
33-45: Consider replacingassertwith explicit null check.The
assert factory != nullon line 41 will be a no-op when assertions are disabled (which is typical in production). If reaching this point without a factory is a programming error that should never occur, consider usingObjects.requireNonNullorPreconditions.checkNotNullto ensure consistent behavior regardless of assertion settings.♻️ Suggested fix
`@Nullable` static HttpConnectionManager unpackConnectionManager(Listener listener, XdsExtensionRegistry registry) { if (listener.getApiListener().hasApiListener()) { final Any apiListener = listener.getApiListener().getApiListener(); final HttpConnectionManagerFactory factory = registry.queryByTypeUrl(apiListener.getTypeUrl(), HttpConnectionManagerFactory.class); - assert factory != null; + if (factory == null) { + throw new IllegalStateException( + "No HttpConnectionManagerFactory registered for type URL: " + + apiListener.getTypeUrl()); + } return factory.create(apiListener, registry.validator()); } return null; }🤖 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/XdsUnpackUtil.java` around lines 33 - 45, In unpackConnectionManager(Listener listener, XdsExtensionRegistry registry) replace the reliance on a Java assert for the HttpConnectionManagerFactory lookup with an explicit null-check and fail-fast behavior: after calling registry.queryByTypeUrl(..., HttpConnectionManagerFactory.class) verify the returned factory is non-null (e.g. Objects.requireNonNull or throw new IllegalStateException with a clear message referencing the typeUrl and listener) before invoking factory.create(apiListener, registry.validator()); this ensures consistent runtime behavior when HttpConnectionManagerFactory is missing.it/xds-client/src/test/java/com/linecorp/armeria/xds/it/XdsControlPlaneErrorHandlingTest.java (1)
90-92: Static shared state across tests - verify isolation.The
version,cache, andnackTrackerare static and shared across all parameterized test executions. WhilesetUpresetsnackTracker, thecacheretains snapshots from prior tests andversionkeeps incrementing. This should be fine since each test sets its own snapshot before assertions, but consider if test ordering or parallel execution could cause flakiness.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@it/xds-client/src/test/java/com/linecorp/armeria/xds/it/XdsControlPlaneErrorHandlingTest.java` around lines 90 - 92, The static shared state (version, cache, nackTracker) can leak between parameterized test runs; fix by ensuring isolation: either make version and cache instance fields instead of static, or reinitialize them in the test setUp method (e.g., version = new AtomicLong(); cache = new SimpleCache<>(node -> GROUP); and call nackTracker.reset() as already done) so each test gets fresh instances; update references to version/cache accordingly and keep nackTracker.reset() in setUp to guarantee a clean state before each test.it/xds-client/src/test/java/com/linecorp/armeria/xds/it/DeltaXdsPreprocessorTest.java (1)
93-95: Minor: Addfinalmodifier for consistency.Other
@RegisterExtensionfields in this class are declaredstatic final. Consider addingfinaltoeventLoopfor consistency.Suggested fix
`@RegisterExtension` `@Order`(3) - static EventLoopExtension eventLoop = new EventLoopExtension(); + static final EventLoopExtension eventLoop = new EventLoopExtension();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@it/xds-client/src/test/java/com/linecorp/armeria/xds/it/DeltaXdsPreprocessorTest.java` around lines 93 - 95, Make the field declaration for the EventLoopExtension consistent by adding the final modifier: change the declaration of the static field eventLoop (type EventLoopExtension) in DeltaXdsPreprocessorTest to be static final so it matches the other `@RegisterExtension` fields in the class.xds/src/main/java/com/linecorp/armeria/xds/ListenerXdsResource.java (1)
116-121: Consider adding Javadoc for future public exposure.The
downstreamFilters()accessor is currently package-private, which is fine. If this method is intended to become public in the future, consider adding Javadoc now.🤖 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/ListenerXdsResource.java` around lines 116 - 121, Add Javadoc to the downstreamFilters() accessor in ListenerXdsResource describing what it returns and any expectations for callers: state that it returns the pre-resolved downstream XdsHttpFilter instances, whether the returned List is mutable/immutable and thread-safety assumptions, include an `@return` tag, and mention nullability (never null) or alternative behavior; keep the method package-private but document that it may be made public in the future so callers understand its contract.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@xds/src/main/java/com/linecorp/armeria/xds/DeltaDiscoveryStub.java`:
- Around line 63-64: The switch default in DeltaDiscoveryStub.basic() (and the
analogous default in SotwDiscoveryStub) throws java.lang.Error for unsupported
XdsType values; change these to throw new IllegalArgumentException("Unexpected
value: " + type) (or similar IllegalArgumentException) so the code uses a proper
checked runtime exception for bad input/enum values; update both
DeltaDiscoveryStub.basic() and the matching method in SotwDiscoveryStub to
replace throw new Error(...) with throw new IllegalArgumentException(...) and
keep the original message text for clarity.
In `@xds/src/main/java/com/linecorp/armeria/xds/TransportSocketFactory.java`:
- Line 2: Revert the copyright header year change in TransportSocketFactory.java
back to the original year; locate the file/class TransportSocketFactory and
restore the previous copyright line (do not update to 2026) so the file keeps
its original header year.
In `@xds/src/main/java/com/linecorp/armeria/xds/XdsBootstrapImpl.java`:
- Around line 51-53: The bootstrap validation is performed after the
DirectoryWatchService is already created, which can leak resources if validation
throws; before creating DirectoryWatchService, run validation by instantiating
XdsResourceValidator and XdsExtensionRegistry and calling
extensionRegistry.assertValid(bootstrap), or if you must keep
DirectoryWatchService creation earlier, ensure any partially initialized state
is closed on validation failure by calling the
DirectoryWatchService.close()/shutdown method in the catch/finally surrounding
extensionRegistry.assertValid(bootstrap); update the code paths that reference
XdsResourceValidator, XdsExtensionRegistry,
extensionRegistry.assertValid(bootstrap), and DirectoryWatchService accordingly.
In `@xds/src/main/java/com/linecorp/armeria/xds/XdsExtensionRegistry.java`:
- Around line 73-79: The register method currently overwrites existing entries
in byName and byTypeUrl which hides duplicate XdsExtensionFactory registrations;
update register(XdsExtensionFactory factory, Map<String,XdsExtensionFactory>
byName, Map<String,XdsExtensionFactory> byTypeUrl) to detect collisions before
inserting: if byName already contains factory.name() or byTypeUrl already
contains any typeUrl from factory.typeUrls(), throw an IllegalStateException (or
a clear RuntimeException) that includes the conflicting name/typeUrl and the
existing and new factory implementations to fail fast rather than silently
overwriting.
In `@xds/src/main/java/com/linecorp/armeria/xds/XdsResourceValidator.java`:
- Around line 76-82: In XdsResourceValidator.unpack(Any message, Class<T> clazz)
add explicit null checks using Objects.requireNonNull for both parameters
(message and clazz) at the top of the method, and when throwing the
IllegalArgumentException on InvalidProtocolBufferException include richer
context (e.g., clazz.getName() and message.getTypeUrl() or other identifying
info) in the exception message so the error clearly identifies what failed to
unpack; update the message passed to Objects.requireNonNull to be a meaningful
description like "message must not be null" / "clazz must not be null".
In `@xds/src/test/java/com/linecorp/armeria/xds/SubscriberStorageTest.java`:
- Around line 68-84: The CapturingWatcher class has plain fields missingType and
missingName that are written in onResourceDoesNotExist (event-loop thread) and
read by Awaitility/test thread, which is racy; make these cross-thread safe by
changing the fields in CapturingWatcher to have proper concurrency semantics
(e.g., declare missingType as volatile XdsType missingType and missingName as
volatile String missingName, or use
AtomicReference<XdsType>/AtomicReference<String>) so that writes in
onResourceDoesNotExist establish a happens-before edge and the test reads see
the updated values.
In `@xds/src/test/java/com/linecorp/armeria/xds/XdsClientIntegrationTest.java`:
- Around line 94-100: The test currently calls
watcher.pollChanged(ClusterSnapshot.class) once, which only consumes a single
queued event and can leave older intermediate ClusterSnapshot events in
watcher.events(), causing flakiness; replace the single poll with a loop that
repeatedly polls/consumes ClusterSnapshot events from the watcher until none
remain, keeping the last non-null ClusterSnapshot as the snapshot to assert
against (still using ClusterSnapshot and watcher.pollChanged/ watcher.events()
APIs), and then assert that the last snapshot.xdsResource().resource() equals
the expected cluster; apply the same draining change to the other similar blocks
that use watcher.pollChanged(ClusterSnapshot.class) (the blocks corresponding to
the other occurrences mentioned).
---
Outside diff comments:
In `@xds/src/main/java/com/linecorp/armeria/xds/XdsStreamSubscriber.java`:
- Around line 56-65: restartTimer() can schedule multiple pending timers because
it doesn't cancel any previously scheduled initialAbsentFuture; modify
restartTimer() to check initialAbsentFuture and cancel it (e.g.,
initialAbsentFuture.cancel(false)) if it's non-null and not done before
assigning a new scheduled future on eventLoop, preserving existing checks for
enableAbsentOnTimeout and retaining the same behavior that clears
initialAbsentFuture and calls onAbsent() when the timeout fires.
---
Nitpick comments:
In
`@it/xds-client/src/test/java/com/linecorp/armeria/xds/it/DeltaXdsPreprocessorTest.java`:
- Around line 93-95: Make the field declaration for the EventLoopExtension
consistent by adding the final modifier: change the declaration of the static
field eventLoop (type EventLoopExtension) in DeltaXdsPreprocessorTest to be
static final so it matches the other `@RegisterExtension` fields in the class.
In
`@it/xds-client/src/test/java/com/linecorp/armeria/xds/it/XdsControlPlaneErrorHandlingTest.java`:
- Around line 90-92: The static shared state (version, cache, nackTracker) can
leak between parameterized test runs; fix by ensuring isolation: either make
version and cache instance fields instead of static, or reinitialize them in the
test setUp method (e.g., version = new AtomicLong(); cache = new
SimpleCache<>(node -> GROUP); and call nackTracker.reset() as already done) so
each test gets fresh instances; update references to version/cache accordingly
and keep nackTracker.reset() in setUp to guarantee a clean state before each
test.
In `@xds/src/main/java/com/linecorp/armeria/xds/CompositeXdsStream.java`:
- Around line 28-38: The constructor of CompositeXdsStream accepts a
streamSupplier without guaranteeing distinct instances per XdsType; to fix,
update the contract and add identity-based deduplication in close(): in
CompositeXdsStream.close() use an identity-based Set (e.g.,
Collections.newSetFromMap(new IdentityHashMap<>())) to track seen XdsStream
instances and call close() only once per unique instance; mention in the
constructor/Javadoc that suppliers should preferably return distinct instances
but the close() protects against accidental reuse of the same instance across
types.
In `@xds/src/main/java/com/linecorp/armeria/xds/ListenerXdsResource.java`:
- Around line 116-121: Add Javadoc to the downstreamFilters() accessor in
ListenerXdsResource describing what it returns and any expectations for callers:
state that it returns the pre-resolved downstream XdsHttpFilter instances,
whether the returned List is mutable/immutable and thread-safety assumptions,
include an `@return` tag, and mention nullability (never null) or alternative
behavior; keep the method package-private but document that it may be made
public in the future so callers understand its contract.
In `@xds/src/main/java/com/linecorp/armeria/xds/SecretXdsResource.java`:
- Around line 41-43: Add a defensive null-check in the SecretXdsResource
constructor: use Objects.requireNonNull on the secret parameter (in
SecretXdsResource(Secret secret, String version, long revision)) and provide a
clear message (e.g., "secret") so construction fails fast; also ensure
java.util.Objects is imported if not already and then assign the checked value
to the instance field 'secret'.
In `@xds/src/main/java/com/linecorp/armeria/xds/XdsUnpackUtil.java`:
- Around line 33-45: In unpackConnectionManager(Listener listener,
XdsExtensionRegistry registry) replace the reliance on a Java assert for the
HttpConnectionManagerFactory lookup with an explicit null-check and fail-fast
behavior: after calling registry.queryByTypeUrl(...,
HttpConnectionManagerFactory.class) verify the returned factory is non-null
(e.g. Objects.requireNonNull or throw new IllegalStateException with a clear
message referencing the typeUrl and listener) before invoking
factory.create(apiListener, registry.validator()); this ensures consistent
runtime behavior when HttpConnectionManagerFactory is missing.
🪄 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: 72f95b20-1467-4d98-b7f9-7b3a38c1b8bf
📒 Files selected for processing (67)
it/xds-client/src/test/java/com/linecorp/armeria/xds/it/ConfigSourceLifecycleObserverTest.javait/xds-client/src/test/java/com/linecorp/armeria/xds/it/DeltaXdsPreprocessorTest.javait/xds-client/src/test/java/com/linecorp/armeria/xds/it/DeltaXdsResourceWatcherTest.javait/xds-client/src/test/java/com/linecorp/armeria/xds/it/XdsControlPlaneErrorHandlingTest.javait/xds-client/src/test/java/com/linecorp/armeria/xds/it/XdsControlPlaneMatrixTest.javatesting-internal/src/main/java/com/linecorp/armeria/internal/testing/InternalTestingBlockHoundIntegration.javaxds/src/main/java/com/linecorp/armeria/xds/AbstractXdsResource.javaxds/src/main/java/com/linecorp/armeria/xds/AdsXdsStream.javaxds/src/main/java/com/linecorp/armeria/xds/ClusterResourceParser.javaxds/src/main/java/com/linecorp/armeria/xds/ClusterXdsResource.javaxds/src/main/java/com/linecorp/armeria/xds/CompositeXdsStream.javaxds/src/main/java/com/linecorp/armeria/xds/ConfigSourceClient.javaxds/src/main/java/com/linecorp/armeria/xds/ConfigSourceLifecycleObserver.javaxds/src/main/java/com/linecorp/armeria/xds/ControlPlaneClientManager.javaxds/src/main/java/com/linecorp/armeria/xds/DefaultConfigSourceLifecycleObserver.javaxds/src/main/java/com/linecorp/armeria/xds/DefaultResponseHandler.javaxds/src/main/java/com/linecorp/armeria/xds/DefaultSubscriptionContext.javaxds/src/main/java/com/linecorp/armeria/xds/DeltaActualStream.javaxds/src/main/java/com/linecorp/armeria/xds/DeltaDiscoveryStub.javaxds/src/main/java/com/linecorp/armeria/xds/EndpointResourceParser.javaxds/src/main/java/com/linecorp/armeria/xds/EndpointXdsResource.javaxds/src/main/java/com/linecorp/armeria/xds/FilterUtil.javaxds/src/main/java/com/linecorp/armeria/xds/HttpConnectionManagerFactory.javaxds/src/main/java/com/linecorp/armeria/xds/ListenerManager.javaxds/src/main/java/com/linecorp/armeria/xds/ListenerResourceParser.javaxds/src/main/java/com/linecorp/armeria/xds/ListenerSnapshot.javaxds/src/main/java/com/linecorp/armeria/xds/ListenerXdsResource.javaxds/src/main/java/com/linecorp/armeria/xds/RawBufferTransportSocketFactory.javaxds/src/main/java/com/linecorp/armeria/xds/ResourceParser.javaxds/src/main/java/com/linecorp/armeria/xds/ResourceStateStore.javaxds/src/main/java/com/linecorp/armeria/xds/RouteEntry.javaxds/src/main/java/com/linecorp/armeria/xds/RouteResourceParser.javaxds/src/main/java/com/linecorp/armeria/xds/RouteStream.javaxds/src/main/java/com/linecorp/armeria/xds/RouteXdsResource.javaxds/src/main/java/com/linecorp/armeria/xds/SecretResourceParser.javaxds/src/main/java/com/linecorp/armeria/xds/SecretXdsResource.javaxds/src/main/java/com/linecorp/armeria/xds/SotwActualStream.javaxds/src/main/java/com/linecorp/armeria/xds/SotwXdsStream.javaxds/src/main/java/com/linecorp/armeria/xds/StateCoordinator.javaxds/src/main/java/com/linecorp/armeria/xds/SubscriberStorage.javaxds/src/main/java/com/linecorp/armeria/xds/SubscriptionContext.javaxds/src/main/java/com/linecorp/armeria/xds/TransportSocketFactory.javaxds/src/main/java/com/linecorp/armeria/xds/TransportSocketStream.javaxds/src/main/java/com/linecorp/armeria/xds/UpstreamTlsTransportSocketFactory.javaxds/src/main/java/com/linecorp/armeria/xds/VirtualHostXdsResource.javaxds/src/main/java/com/linecorp/armeria/xds/XdsBootstrapImpl.javaxds/src/main/java/com/linecorp/armeria/xds/XdsConverterUtil.javaxds/src/main/java/com/linecorp/armeria/xds/XdsExtensionFactory.javaxds/src/main/java/com/linecorp/armeria/xds/XdsExtensionRegistry.javaxds/src/main/java/com/linecorp/armeria/xds/XdsResource.javaxds/src/main/java/com/linecorp/armeria/xds/XdsResourceException.javaxds/src/main/java/com/linecorp/armeria/xds/XdsResourceValidator.javaxds/src/main/java/com/linecorp/armeria/xds/XdsResponseHandler.javaxds/src/main/java/com/linecorp/armeria/xds/XdsStreamSubscriber.javaxds/src/main/java/com/linecorp/armeria/xds/XdsUnpackUtil.javaxds/src/main/java/com/linecorp/armeria/xds/XdsValidatorIndexRegistry.javaxds/src/main/java/com/linecorp/armeria/xds/client/endpoint/RouterFilterFactory.javaxds/src/main/java/com/linecorp/armeria/xds/filter/HttpFilterFactory.javaxds/src/main/java/com/linecorp/armeria/xds/filter/HttpFilterFactoryRegistry.javaxds/src/test/java/com/linecorp/armeria/xds/SotwXdsStreamTest.javaxds/src/test/java/com/linecorp/armeria/xds/StateCoordinatorTest.javaxds/src/test/java/com/linecorp/armeria/xds/SubscriberStorageTest.javaxds/src/test/java/com/linecorp/armeria/xds/TestResourceWatcher.javaxds/src/test/java/com/linecorp/armeria/xds/XdsClientIntegrationTest.javaxds/src/test/java/com/linecorp/armeria/xds/XdsExtensionRegistryTest.javaxds/src/test/java/com/linecorp/armeria/xds/XdsResourceValidatorTest.javaxds/src/test/java/com/linecorp/armeria/xds/XdsValidatorIndexRegistryTest.java
💤 Files with no reviewable changes (8)
- xds/src/test/java/com/linecorp/armeria/xds/XdsValidatorIndexRegistryTest.java
- xds/src/main/java/com/linecorp/armeria/xds/filter/HttpFilterFactoryRegistry.java
- xds/src/main/java/com/linecorp/armeria/xds/DefaultResponseHandler.java
- xds/src/main/java/com/linecorp/armeria/xds/XdsConverterUtil.java
- xds/src/main/java/com/linecorp/armeria/xds/XdsValidatorIndexRegistry.java
- xds/src/main/java/com/linecorp/armeria/xds/XdsResponseHandler.java
- xds/src/main/java/com/linecorp/armeria/xds/SotwXdsStream.java
- xds/src/test/java/com/linecorp/armeria/xds/SotwXdsStreamTest.java
Comment on lines
+63
to
+64
| default: | ||
| throw new Error("Unexpected value: " + type); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n --type=java 'throw new Error\("Unexpected value: "'
echo '---'
rg -n --type=java '\bDeltaDiscoveryStub\.basic\s*\('
echo '---'
rg -n --type=java 'discoverableTypes\s*\(|enum\s+XdsType\b' xds/src/main/java/com/linecorp/armeria/xdsRepository: line/armeria
Length of output: 1180
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check the full switch statement in DeltaDiscoveryStub.java
echo "=== DeltaDiscoveryStub.java context (lines 55-70) ==="
sed -n '55,70p' xds/src/main/java/com/linecorp/armeria/xds/DeltaDiscoveryStub.java
echo ""
echo "=== SotwDiscoveryStub.java context (lines 55-70) ==="
sed -n '55,70p' xds/src/main/java/com/linecorp/armeria/xds/SotwDiscoveryStub.java
echo ""
echo "=== XdsType.java enum values and discoverableTypes() method ==="
sed -n '28,60p' xds/src/main/java/com/linecorp/armeria/xds/XdsType.java
echo ""
echo "=== ConfigSourceClient.java context (lines 100-115) ==="
sed -n '100,115p' xds/src/main/java/com/linecorp/armeria/xds/ConfigSourceClient.javaRepository: line/armeria
Length of output: 3279
Fix Error exception in both DeltaDiscoveryStub and SotwDiscoveryStub for unsupported XdsType values.
Both DeltaDiscoveryStub.basic() (line 64) and SotwDiscoveryStub (same pattern at line 64) throw Error when encountering an unsupported XdsType. This is inappropriate—throwing Error is reserved for JVM-fatal conditions, not for bad input or unsupported enum values. Throwing Error makes normal error handling much harder and can unnecessarily crash the client.
Replace with IllegalArgumentException:
Suggested fix (both files)
default:
- throw new Error("Unexpected value: " + type);
+ throw new IllegalArgumentException("Unsupported xDS type: " + type);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| default: | |
| throw new Error("Unexpected value: " + type); | |
| default: | |
| throw new IllegalArgumentException("Unsupported xDS type: " + type); |
🤖 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/DeltaDiscoveryStub.java` around
lines 63 - 64, The switch default in DeltaDiscoveryStub.basic() (and the
analogous default in SotwDiscoveryStub) throws java.lang.Error for unsupported
XdsType values; change these to throw new IllegalArgumentException("Unexpected
value: " + type) (or similar IllegalArgumentException) so the code uses a proper
checked runtime exception for bad input/enum values; update both
DeltaDiscoveryStub.basic() and the matching method in SotwDiscoveryStub to
replace throw new Error(...) with throw new IllegalArgumentException(...) and
keep the original message text for clarity.
| @@ -1,5 +1,5 @@ | |||
| /* | |||
| * Copyright 2025 LY Corporation | |||
| * Copyright 2026 LY Corporation | |||
Contributor
There was a problem hiding this comment.
Keep the existing header year.
This file already existed, so bumping the copyright year here just adds avoidable churn. Please keep the original year.
Suggested fix
- * Copyright 2026 LY Corporation
+ * Copyright 2025 LY CorporationAs per coding guidelines, "keep the copyright header year stable (avoid changing $today.year across many files)".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| * Copyright 2026 LY Corporation | |
| * Copyright 2025 LY Corporation |
🤖 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/TransportSocketFactory.java` at
line 2, Revert the copyright header year change in TransportSocketFactory.java
back to the original year; locate the file/class TransportSocketFactory and
restore the previous copyright line (do not update to 2026) so the file keeps
its original header year.
Comment on lines
+51
to
+53
| final XdsResourceValidator resourceValidator = new XdsResourceValidator(); | ||
| final XdsExtensionRegistry extensionRegistry = XdsExtensionRegistry.of(resourceValidator); | ||
| extensionRegistry.assertValid(bootstrap); |
Contributor
There was a problem hiding this comment.
Avoid validating after allocating the watch service.
This adds a new constructor-time throw path, but DirectoryWatchService has already been created above. If bootstrap validation fails here, the watch-service resources leak. Move the allocation below validation or close partially initialized state on failure.
🤖 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/XdsBootstrapImpl.java` around
lines 51 - 53, The bootstrap validation is performed after the
DirectoryWatchService is already created, which can leak resources if validation
throws; before creating DirectoryWatchService, run validation by instantiating
XdsResourceValidator and XdsExtensionRegistry and calling
extensionRegistry.assertValid(bootstrap), or if you must keep
DirectoryWatchService creation earlier, ensure any partially initialized state
is closed on validation failure by calling the
DirectoryWatchService.close()/shutdown method in the catch/finally surrounding
extensionRegistry.assertValid(bootstrap); update the code paths that reference
XdsResourceValidator, XdsExtensionRegistry,
extensionRegistry.assertValid(bootstrap), and DirectoryWatchService accordingly.
Comment on lines
+73
to
+79
| private static void register(XdsExtensionFactory factory, | ||
| Map<String, XdsExtensionFactory> byName, | ||
| Map<String, XdsExtensionFactory> byTypeUrl) { | ||
| byName.put(factory.name(), factory); | ||
| for (String typeUrl : factory.typeUrls()) { | ||
| byTypeUrl.put(typeUrl, factory); | ||
| } |
Contributor
There was a problem hiding this comment.
Reject duplicate extension names and type URLs during registration.
These put(...) calls silently overwrite an earlier factory. With SPI providers plus built-ins, the winner now depends on registration order, and a colliding extension can become unreachable without any signal.
Suggested fix
private static void register(XdsExtensionFactory factory,
Map<String, XdsExtensionFactory> byName,
Map<String, XdsExtensionFactory> byTypeUrl) {
- byName.put(factory.name(), factory);
+ final XdsExtensionFactory previousByName = byName.putIfAbsent(factory.name(), factory);
+ if (previousByName != null) {
+ throw new IllegalArgumentException(
+ "Duplicate xDS extension name '" + factory.name() + "': " +
+ previousByName.getClass().getName() + " vs " + factory.getClass().getName());
+ }
for (String typeUrl : factory.typeUrls()) {
- byTypeUrl.put(typeUrl, factory);
+ final XdsExtensionFactory previousByTypeUrl = byTypeUrl.putIfAbsent(typeUrl, factory);
+ if (previousByTypeUrl != null) {
+ throw new IllegalArgumentException(
+ "Duplicate xDS extension type URL '" + typeUrl + "': " +
+ previousByTypeUrl.getClass().getName() + " vs " +
+ factory.getClass().getName());
+ }
}
}🤖 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/XdsExtensionRegistry.java` around
lines 73 - 79, The register method currently overwrites existing entries in
byName and byTypeUrl which hides duplicate XdsExtensionFactory registrations;
update register(XdsExtensionFactory factory, Map<String,XdsExtensionFactory>
byName, Map<String,XdsExtensionFactory> byTypeUrl) to detect collisions before
inserting: if byName already contains factory.name() or byTypeUrl already
contains any typeUrl from factory.typeUrls(), throw an IllegalStateException (or
a clear RuntimeException) that includes the conflicting name/typeUrl and the
existing and new factory implementations to fail fast rather than silently
overwriting.
Comment on lines
+76
to
+82
| public <T extends Message> T unpack(Any message, Class<T> clazz) { | ||
| final T unpacked; | ||
| try { | ||
| unpacked = message.unpack(clazz); | ||
| } catch (InvalidProtocolBufferException e) { | ||
| throw new IllegalArgumentException("Error unpacking: " + clazz.getName(), e); | ||
| } |
Contributor
There was a problem hiding this comment.
Add explicit null-parameter checks in public unpack API.
unpack(...) currently relies on downstream NPEs. Please validate inputs up front and include richer unpack context in the exception message.
Proposed fix
import java.util.Comparator;
+import java.util.Objects;
import java.util.ServiceLoader;
@@
public <T extends Message> T unpack(Any message, Class<T> clazz) {
+ Objects.requireNonNull(message, "message");
+ Objects.requireNonNull(clazz, "clazz");
final T unpacked;
try {
unpacked = message.unpack(clazz);
} catch (InvalidProtocolBufferException e) {
- throw new IllegalArgumentException("Error unpacking: " + clazz.getName(), e);
+ throw new IllegalArgumentException(
+ "Failed to unpack Any[typeUrl=" + message.getTypeUrl() + "] as " + clazz.getName(), e);
}
assertValid(unpacked);
return unpacked;
}As per coding guidelines, "follow LY OSS style (null checks via Objects.requireNonNull ...); use meaningful exception messages and validate public method parameters with detailed messages."
🤖 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/XdsResourceValidator.java` around
lines 76 - 82, In XdsResourceValidator.unpack(Any message, Class<T> clazz) add
explicit null checks using Objects.requireNonNull for both parameters (message
and clazz) at the top of the method, and when throwing the
IllegalArgumentException on InvalidProtocolBufferException include richer
context (e.g., clazz.getName() and message.getTypeUrl() or other identifying
info) in the exception message so the error clearly identifies what failed to
unpack; update the message passed to Objects.requireNonNull to be a meaningful
description like "message must not be null" / "clazz must not be null".
Comment on lines
+68
to
+84
| await().atMost(1, TimeUnit.SECONDS) | ||
| .untilAsserted(() -> assertThat(watcher.missingType).isEqualTo(XdsType.ROUTE)); | ||
| assertThat(watcher.missingName).isEqualTo(ROUTE_NAME); | ||
| } | ||
|
|
||
| private static final class CapturingWatcher implements ResourceWatcher<XdsResource> { | ||
| private XdsType missingType; | ||
| private String missingName; | ||
|
|
||
| @Override | ||
| public void onChanged(XdsResource update) {} | ||
|
|
||
| @Override | ||
| public void onResourceDoesNotExist(XdsType type, String resourceName) { | ||
| missingType = type; | ||
| missingName = resourceName; | ||
| } |
Contributor
There was a problem hiding this comment.
Make the timeout watcher state cross-thread safe.
onResourceDoesNotExist() runs on the event loop, but Awaitility reads missingType and missingName from the test thread. With plain fields there is no happens-before edge, so this can flake, and the post-await() assertion on missingName is especially racy.
Suggested fix
- await().atMost(1, TimeUnit.SECONDS)
- .untilAsserted(() -> assertThat(watcher.missingType).isEqualTo(XdsType.ROUTE));
- assertThat(watcher.missingName).isEqualTo(ROUTE_NAME);
+ await().atMost(1, TimeUnit.SECONDS)
+ .untilAsserted(() -> {
+ assertThat(watcher.missingType).isEqualTo(XdsType.ROUTE);
+ assertThat(watcher.missingName).isEqualTo(ROUTE_NAME);
+ });
@@
- private XdsType missingType;
- private String missingName;
+ private volatile XdsType missingType;
+ private volatile String missingName;Based on learnings, xDS callbacks in this module run on the event loop, so this watcher is written from the event-loop thread and read from the test thread.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| await().atMost(1, TimeUnit.SECONDS) | |
| .untilAsserted(() -> assertThat(watcher.missingType).isEqualTo(XdsType.ROUTE)); | |
| assertThat(watcher.missingName).isEqualTo(ROUTE_NAME); | |
| } | |
| private static final class CapturingWatcher implements ResourceWatcher<XdsResource> { | |
| private XdsType missingType; | |
| private String missingName; | |
| @Override | |
| public void onChanged(XdsResource update) {} | |
| @Override | |
| public void onResourceDoesNotExist(XdsType type, String resourceName) { | |
| missingType = type; | |
| missingName = resourceName; | |
| } | |
| await().atMost(1, TimeUnit.SECONDS) | |
| .untilAsserted(() -> { | |
| assertThat(watcher.missingType).isEqualTo(XdsType.ROUTE); | |
| assertThat(watcher.missingName).isEqualTo(ROUTE_NAME); | |
| }); | |
| } | |
| private static final class CapturingWatcher implements ResourceWatcher<XdsResource> { | |
| private volatile XdsType missingType; | |
| private volatile String missingName; | |
| `@Override` | |
| public void onChanged(XdsResource update) {} | |
| `@Override` | |
| public void onResourceDoesNotExist(XdsType type, String resourceName) { | |
| missingType = type; | |
| missingName = resourceName; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@xds/src/test/java/com/linecorp/armeria/xds/SubscriberStorageTest.java` around
lines 68 - 84, The CapturingWatcher class has plain fields missingType and
missingName that are written in onResourceDoesNotExist (event-loop thread) and
read by Awaitility/test thread, which is racy; make these cross-thread safe by
changing the fields in CapturingWatcher to have proper concurrency semantics
(e.g., declare missingType as volatile XdsType missingType and missingName as
volatile String missingName, or use
AtomicReference<XdsType>/AtomicReference<String>) so that writes in
onResourceDoesNotExist establish a happens-before edge and the test reads see
the updated values.
Comment on lines
+94
to
100
| await().untilAsserted(() -> { | ||
| final ClusterSnapshot snapshot = watcher.pollChanged(ClusterSnapshot.class); | ||
| assertThat(snapshot).isNotNull(); | ||
| final Cluster expectedCluster2 = cache.getSnapshot(GROUP).clusters() | ||
| .resources().get(clusterName); | ||
| assertThat(last.xdsResource().resource()).isEqualTo(expectedCluster2); | ||
| assertThat(snapshot.xdsResource().resource()).isEqualTo(expectedCluster2); | ||
| }); |
Contributor
There was a problem hiding this comment.
Drain all queued ClusterSnapshot updates before asserting.
pollChanged() removes only one event. If a single cache update enqueues more than one ClusterSnapshot, the extra snapshots stay in watcher.events(), which can make the later emptiness checks flaky or cause retries to assert against an intermediate state. These assertions should drain pending ClusterSnapshots and verify the latest one instead.
Also applies to: 138-144, 164-172, 229-237
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@xds/src/test/java/com/linecorp/armeria/xds/XdsClientIntegrationTest.java`
around lines 94 - 100, The test currently calls
watcher.pollChanged(ClusterSnapshot.class) once, which only consumes a single
queued event and can leave older intermediate ClusterSnapshot events in
watcher.events(), causing flakiness; replace the single poll with a loop that
repeatedly polls/consumes ClusterSnapshot events from the watcher until none
remain, keeping the last non-null ClusterSnapshot as the snapshot to assert
against (still using ClusterSnapshot and watcher.pollChanged/ watcher.events()
APIs), and then assert that the last snapshot.xdsResource().resource() equals
the expected cluster; apply the same draining change to the other similar blocks
that use watcher.pollChanged(ClusterSnapshot.class) (the blocks corresponding to
the other occurrences mentioned).
Contributor
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
xds/src/main/java/com/linecorp/armeria/xds/XdsExtensionRegistry.java (1)
51-52: Remove misleading@VisibleForTestingannotation fromof(...)method.Line 52: This method is called from
XdsBootstrapImpl(production code) during normal bootstrap initialization, not just from tests. The@VisibleForTestingannotation contradicts this usage and should be removed.🤖 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/XdsExtensionRegistry.java` around lines 51 - 52, The `@VisibleForTesting` annotation on the static method XdsExtensionRegistry.of(XdsResourceValidator) is misleading because the method is used by production code (XdsBootstrapImpl); remove the `@VisibleForTesting` annotation from the of(...) method in class XdsExtensionRegistry so its visibility correctly reflects production usage and no test-only marker remains.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@xds/src/main/java/com/linecorp/armeria/xds/XdsExtensionRegistry.java`:
- Around line 51-52: The `@VisibleForTesting` annotation on the static method
XdsExtensionRegistry.of(XdsResourceValidator) is misleading because the method
is used by production code (XdsBootstrapImpl); remove the `@VisibleForTesting`
annotation from the of(...) method in class XdsExtensionRegistry so its
visibility correctly reflects production usage and no test-only marker remains.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 326d66f6-6d7d-42a2-a753-99c8ebafeae3
📒 Files selected for processing (1)
xds/src/main/java/com/linecorp/armeria/xds/XdsExtensionRegistry.java
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
1 participant
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.
This PR is a subset of #6721
It should be reviewed after #6709, the relevant commit is: ecf13d3
Motivation
The xDS module previously scattered extension handling (HTTP filters, transport sockets, network filters) across multiple static registries and utility classes (
HttpFilterFactoryRegistry,XdsValidatorIndexRegistry,XdsConverterUtil), each with inconsistent lookup and validation behavior. Validation was also performed redundantly — both during resource parsing and in individualXdsResourceconstructors — which could be expensive due to full message tree traversal.This PR introduces
XdsExtensionRegistryas the single, bootstrap-scoped registry that unifies extension factory lookup, protoAnyunpacking, and validation.Modifications
Introduce
XdsExtensionRegistryto unify unpack/validation behaviorXdsExtensionFactoryas the base interface for all extension factories, providingname()andtypeUrls()for dual-key resolution (type URL primary, extension name fallback).XdsExtensionRegistryas the central registry that discovers factories via SPI and built-in registration. Providesquery()for factory lookup,unpack()forAnyfields, andassertValid()for proto validation.XdsBootstrapImpland threaded through the pipeline viaSubscriptionContext, rather than being a static singleton. This allows users to inject custom factories via the bootstrap builder in the future.HttpFilterFactoryRegistryandXdsValidatorIndexRegistrywhich are now subsumed byXdsExtensionRegistry.Consolidate validation into three well-defined points
XdsResourceValidatorwhich delegates to the highest-priorityXdsValidatorIndexloaded via SPI. Validation now happens at exactly three points:Bootstrapproto is validated once at construction time.ResourceParser.parseResources()/parseDeltaResources().Anyfields are validated when unpacked viaregistry.unpack().ClusterXdsResource,ListenerXdsResource,EndpointXdsResource,RouteXdsResource,SecretXdsResource, andVirtualHostXdsResourceconstructors. This avoids redundant full-message-tree traversals.XdsConverterUtilwhose config source validation logic is subsumed by the centralized validation.HTTP filters now use the extension registry
HttpFilterFactorynow extendsXdsExtensionFactory;create()receivesXdsResourceValidatorfor config unpacking.RouterFilterFactoryimplementsname()/typeUrls()and now properly unpacks theRouterproto. ExtractedRouterXdsHttpFilteras a public inner class with arouter()getter.FilterUtillookups changed fromHttpFilterFactoryRegistrytoXdsExtensionRegistry.query().Transport sockets now use the extension registry
TransportSocketFactoryas the extension factory interface for transport sockets.UpstreamTlsTransportSocketFactorywhich encapsulates the TLS logic previously hardcoded inTransportSocketStream.RawBufferTransportSocketFactoryas a simple pass-through for plaintext transport sockets.TransportSocketStreamfrom monolithic TLS handling to a strategy pattern — queries the registry for the appropriateTransportSocketFactoryand delegates creation.Listener unpacking centralized via
XdsUnpackUtilXdsUnpackUtilto centralize unpacking ofHttpConnectionManagerfrom listener configs and downstream filter resolution.HttpConnectionManagerFactoryfor parsingHttpConnectionManagernetwork filter configs.ListenerResourceParser/ListenerXdsResource: Downstream filters are resolved at parse time viaXdsUnpackUtiland stored as a field, rather than computed on demand.Result
XdsExtensionRegistrywith dual-key lookup (type URL + name).