Skip to content

Replace ResourceWatcher with SnapshotWatcher#6796

Open
jrhee17 wants to merge 2 commits into
line:mainfrom
jrhee17:pr/snapshot-watcher
Open

Replace ResourceWatcher with SnapshotWatcher#6796
jrhee17 wants to merge 2 commits into
line:mainfrom
jrhee17:pr/snapshot-watcher

Conversation

@jrhee17
Copy link
Copy Markdown
Contributor

@jrhee17 jrhee17 commented Jun 5, 2026

Motivation:

Simplify the internal xDS resource watcher contract from three callbacks (onChanged, onError, onResourceDoesNotExist) to a single onUpdate(@Nullable T, @Nullable Throwable) by replacing ResourceWatcher/XdsStreamSubscriber with the existing SnapshotWatcher/new CompositeSnapshotWatcher.

Modifications:

  • Deleted ResourceWatcher, XdsStreamSubscriber, and DummyResourceWatcher
  • Added CompositeSnapshotWatcher as a drop-in replacement for XdsStreamSubscriber
  • Updated SubscriberStorage, StateCoordinator, ConfigSourceHandler, ResourceNode, and ResourceNodeAdapter to use the new types
  • Updated StateCoordinatorTest and SubscriberStorageTest to use SnapshotWatcher lambdas

Result:

  • No user-facing API changes — all modified types are package-private.

@jrhee17 jrhee17 added this to the 1.40.0 milestone Jun 5, 2026
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Jun 5, 2026

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 891457e7-fe75-4311-a66a-bd11a29fe501

📥 Commits

Reviewing files that changed from the base of the PR and between 7dc92fc and eda0cf0.

📒 Files selected for processing (2)
  • xds/src/main/java/com/linecorp/armeria/xds/ConfigSourceHandler.java
  • xds/src/main/java/com/linecorp/armeria/xds/StateCoordinator.java
💤 Files with no reviewable changes (1)
  • xds/src/main/java/com/linecorp/armeria/xds/StateCoordinator.java
🚧 Files skipped from review as they are similar to previous changes (1)
  • xds/src/main/java/com/linecorp/armeria/xds/ConfigSourceHandler.java

📝 Walkthrough

Walkthrough

Migrates xDS subscription from stream-style callbacks (ResourceWatcher, XdsStreamSubscriber) to snapshot-style SnapshotWatcher with a CompositeSnapshotWatcher fan-out and updates storage, coordinator, adapters, and tests to use onUpdate(value, error).

Changes

XDS Watcher Architecture Refactor

Layer / File(s) Summary
Composite snapshot watcher implementation
xds/src/main/java/com/linecorp/armeria/xds/CompositeSnapshotWatcher.java
Introduces CompositeSnapshotWatcher<T> that schedules optional absent-on-timeout, cancels timers on update, manages downstream SnapshotWatcher registration, fans out onUpdate(value,error) to all watchers, and logs individual watcher exceptions.
Watcher interface contract migration
xds/src/main/java/com/linecorp/armeria/xds/ResourceNode.java, xds/src/main/java/com/linecorp/armeria/xds/ResourceNodeAdapter.java, xds/src/main/java/com/linecorp/armeria/xds/ConfigSourceHandler.java
ResourceNode now extends SnapshotWatcher<T>; ResourceNodeAdapter implements single onUpdate(@nullableT value,@nullable Throwable error) with conditional metric routing; ConfigSourceHandler methods accept SnapshotWatcher<? extends XdsResource> instead of ResourceWatcher.
Subscriber storage and coordination
xds/src/main/java/com/linecorp/armeria/xds/SubscriberStorage.java, xds/src/main/java/com/linecorp/armeria/xds/StateCoordinator.java
SubscriberStorage stores CompositeSnapshotWatcher instances and wires per-subscriber addWatcher/removeWatcher. StateCoordinator updates register/unregister signatures to SnapshotWatcher<T> and rewrites delivery/replay to call onUpdate(resource, null) or onUpdate(null, exception). XdsStreamSubscriber is removed.
Test updates for snapshot watcher API
xds/src/test/java/com/linecorp/armeria/xds/StateCoordinatorTest.java, xds/src/test/java/com/linecorp/armeria/xds/SubscriberStorageTest.java
Replaces bespoke watcher helpers with inline SnapshotWatcher lambdas and AtomicReference captures; reorganizes tests to validate immediate delivery, cached replay on late registration, absence-after-missing behavior, duplicate-register semantics, and timeout error capture. The DummyResourceWatcher helper is removed.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

  • line/armeria#6608: Refactors SnapshotWatcher to use unified onUpdate(snapshot, error) callback.
  • line/armeria#6747: Changes ConfigSourceHandler-based subscription wiring that overlaps with this PR's subscription boundary updates.
  • line/armeria#6709: Related StateCoordinator/subscriber plumbing changes that touch similar watcher integration points.

Suggested reviewers

  • trustin
  • ikhoon
  • minwoox

"From my rabbit burrow I peep and say with cheer,
Watchers gathered closely now — no streams to steer.
One snapshot voice for many, small errors ignored,
Composite fans the signal — the meadow sings toward! 🐇✨"

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 5.56% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Replace ResourceWatcher with SnapshotWatcher' accurately and concisely summarizes the primary change: migrating from one watcher interface to another.
Description check ✅ Passed The description comprehensively explains the motivation, modifications, and results, clearly relating to all aspects of the changeset.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov
Copy link
Copy Markdown

codecov Bot commented Jun 5, 2026

Codecov Report

❌ Patch coverage is 92.85714% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 75.00%. Comparing base (8150425) to head (eda0cf0).
⚠️ Report is 482 commits behind head on main.

Files with missing lines Patch % Lines
...linecorp/armeria/xds/CompositeSnapshotWatcher.java 90.00% 2 Missing and 1 partial ⚠️
.../com/linecorp/armeria/xds/ResourceNodeAdapter.java 85.71% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #6796      +/-   ##
============================================
+ Coverage     74.46%   75.00%   +0.54%     
- Complexity    22234    24914    +2680     
============================================
  Files          1963     2214     +251     
  Lines         82437    92667   +10230     
  Branches      10764    12106    +1342     
============================================
+ Hits          61385    69509    +8124     
- Misses        15918    17370    +1452     
- Partials       5134     5788     +654     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@jrhee17 jrhee17 marked this pull request as ready for review June 5, 2026 08:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant