Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughRefactored XDS reactive streams to use eager switching semantics, introduced a ResourceNodeMeterBinderFactory for metric binder lifecycle and reference counting, and removed centralized resource caching by eliminating cache updates and storage usages. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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)
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.
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/ResourceNodeAdapter.java (1)
27-36:⚠️ Potential issue | 🟠 MajorReacquire the pooled meter binder on every start.
Line 36 increments the shared binder ref count once in the constructor, but Line 84 decrements it every time this
RefCountedStreamstops. Because the stream can start again after its watcher count returns to zero, a stop → start → stop cycle on the sameResourceNodeAdapterwill release a reference it never reacquired and can remove the shared meters while another adapter for the same(type, name)is still active. The same mismatch also leaks a reference ifcontext.subscribe(this)throws before the cleanup lambda is installed. Please moveacquire()intoonStart()(beforecontext.subscribe(this)), close it in the catch path, and release it aftercontext.unsubscribe(this).Also applies to: 73-85
🤖 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/ResourceNodeAdapter.java` around lines 27 - 36, The constructor currently acquires the shared ResourceNodeMeterBinder once (resourceNodeMeterBinder) but never reacquires it on subsequent restarts, causing ref-count mismatch; move the acquire call into onStart() (i.e., call context.meterBinderFactory().acquire(type, name) at the start of onStart() before calling context.subscribe(this)), ensure you close it in the onStart() catch path if subscribe throws, and release it after context.unsubscribe(this) when stopping the RefCountedStream (and in any stop/cleanup paths) so each start acquires and every corresponding stop releases the binder; update the handling around context.subscribe(this)/context.unsubscribe(this) and the stop lambda to use this per-start acquisition and proper cleanup of the ResourceNodeMeterBinder.
🧹 Nitpick comments (1)
xds/src/main/java/com/linecorp/armeria/xds/SubscriberStorage.java (1)
54-55: Complete the ResourceCache cleanup to avoid stale in-file abstractions.After removing cache wiring from subscriber construction,
SubscriberStorage.ResourceCacheis now a leftover abstraction in this file. Removing it (and its now-only supporting imports) would make the cleanup consistent and reduce confusion.♻️ Proposed cleanup
-import com.google.common.collect.ImmutableMap; - -import com.linecorp.armeria.common.annotation.Nullable; @@ - static class ResourceCache { - - private final Map<XdsType, Map<String, Object>> type2resources = new HashMap<>(); - - void updateResources(XdsType type, Map<String, Object> resources) { - type2resources.put(type, resources); - } - - `@Nullable` - Object find(XdsType type, String resourceName) { - return type2resources.getOrDefault(type, ImmutableMap.of()) - .get(resourceName); - } - } }🤖 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/SubscriberStorage.java` around lines 54 - 55, Remove the now-unused ResourceCache abstraction from SubscriberStorage: delete the inner class SubscriberStorage.ResourceCache and any imports that only supported it, and update code references so subscriber construction uses the existing subscriberMap logic (e.g., where XdsStreamSubscriber is created and subscriberMap.get(type).put(resourceName, subscriber)); ensure no remaining references to ResourceCache remain and run/adjust imports accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@xds/src/main/java/com/linecorp/armeria/xds/ResourceNodeAdapter.java`:
- Around line 27-36: The constructor currently acquires the shared
ResourceNodeMeterBinder once (resourceNodeMeterBinder) but never reacquires it
on subsequent restarts, causing ref-count mismatch; move the acquire call into
onStart() (i.e., call context.meterBinderFactory().acquire(type, name) at the
start of onStart() before calling context.subscribe(this)), ensure you close it
in the onStart() catch path if subscribe throws, and release it after
context.unsubscribe(this) when stopping the RefCountedStream (and in any
stop/cleanup paths) so each start acquires and every corresponding stop releases
the binder; update the handling around
context.subscribe(this)/context.unsubscribe(this) and the stop lambda to use
this per-start acquisition and proper cleanup of the ResourceNodeMeterBinder.
---
Nitpick comments:
In `@xds/src/main/java/com/linecorp/armeria/xds/SubscriberStorage.java`:
- Around line 54-55: Remove the now-unused ResourceCache abstraction from
SubscriberStorage: delete the inner class SubscriberStorage.ResourceCache and
any imports that only supported it, and update code references so subscriber
construction uses the existing subscriberMap logic (e.g., where
XdsStreamSubscriber is created and subscriberMap.get(type).put(resourceName,
subscriber)); ensure no remaining references to ResourceCache remain and
run/adjust imports accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2e51b27d-5688-458c-beed-6abe02c63c52
📒 Files selected for processing (15)
xds/src/main/java/com/linecorp/armeria/xds/ClusterStream.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/ListenerStream.javaxds/src/main/java/com/linecorp/armeria/xds/ResourceNodeAdapter.javaxds/src/main/java/com/linecorp/armeria/xds/ResourceNodeMeterBinder.javaxds/src/main/java/com/linecorp/armeria/xds/ResourceNodeMeterBinderFactory.javaxds/src/main/java/com/linecorp/armeria/xds/RouteStream.javaxds/src/main/java/com/linecorp/armeria/xds/SnapshotStream.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/SwitchMapEagerStream.javaxds/src/main/java/com/linecorp/armeria/xds/TransportSocketStream.javaxds/src/main/java/com/linecorp/armeria/xds/XdsStreamSubscriber.javaxds/src/test/java/com/linecorp/armeria/xds/StreamSwitchMapEagerTest.java
💤 Files with no reviewable changes (2)
- xds/src/main/java/com/linecorp/armeria/xds/DefaultResponseHandler.java
- xds/src/main/java/com/linecorp/armeria/xds/ResourceNodeMeterBinder.java
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6704 +/- ##
============================================
- 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 Sentry. 🚀 New features to boost your workflow:
|
Contributor
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.
This PR is a subset of #6700
Motivation
The xDS resource stream layer used a lazy
switchMapoperator when mapping an upstream resourceto a downstream snapshot stream (e.g., a
ListenerXdsResource→ListenerSnapshot). On everyupstream change, the lazy implementation would unsubscribe from the old inner stream before
creating and subscribing to the new one. This caused the entire downstream
ResourceNodechain— including SDS secret subscriptions — to be torn down and rebuilt on every xDS update.
This is inconsistent with Envoy's behavior, which
maintains existing resource watchers across xDS updates and only replaces them when a genuinely
different resource is received.
A secondary consequence was that
ResourceNodeMeterBinder— which records revision, error, andmissing-resource metrics per resource node — was allocated and deregistered per
ResourceNodeAdapterinstance. With the old lazy teardown, gauges and counters reset to zero onevery xDS update, producing noisy metric churn.
Modifications
Replace
SwitchMapStream(lazy) withSwitchMapEagerStream(eager): the new operatorsubscribes to the new inner stream before closing the old one, so downstream resource
subscriptions survive across upstream changes. An epoch counter discards events from superseded
inner streams.
SwitchMapStreamis deleted.SnapshotStream:switchMap()→switchMapEager(), backed bySwitchMapEagerStreamClusterStream,TransportSocketStream,ListenerStream,RouteStreamReplace
ResourceNodeMeterBinderwithResourceNodeMeterBinderFactory: the factory maintainsa reference-counted map of binder instances keyed by
(XdsType, resourceName).acquire()returns the shared binder (incrementing the ref count);
close()only deregisters meters whenthe last reference drops to zero, preventing metric resets during the brief overlap when two
ResourceNodeAdapterinstances exist for the same resource during an eager switch.ResourceNodeMeterBinderdeleted (moved as inner class ofResourceNodeMeterBinderFactory)ResourceNodeMeterBinderFactoryadded; exposed viaSubscriptionContext.meterBinderFactory()DefaultSubscriptionContext: constructs and holds the factoryResourceNodeAdapter: callscontext.meterBinderFactory().acquire()on construction,meterBinder.close()on unsubscriptionSubscriberStorage#ResourceCacheis cleaned upResult
subscriptions, consistent with Envoy's resource-cache behavior.
resource.node.revision,resource.node.error,resource.node.missing) remain continuous across xDS updates and only deregister when allwatchers for a given resource name have closed.