Skip to content

feat(etcdv3): support unsubscribe and subscription recovery#3387

Open
leno23 wants to merge 9 commits into
apache:developfrom
leno23:codex/etcdv3-subscription-recovery-3366
Open

feat(etcdv3): support unsubscribe and subscription recovery#3387
leno23 wants to merge 9 commits into
apache:developfrom
leno23:codex/etcdv3-subscription-recovery-3366

Conversation

@leno23

@leno23 leno23 commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

Description

Fixes #3366.

This PR fills the missing etcdv3 registry subscription lifecycle behavior:

  • implements DoUnregister by deleting the registered etcd key after validating the client
  • replaces the single shared config listener with per-service subscribed listeners
  • implements DoUnsubscribe by closing and removing the service listener
  • preserves active subscriptions when listeners are reinitialized after client restart
  • dispatches data changes to every matching subscribed service key

Validation

  • go test ./registry/etcdv3
  • gofmt on touched files
  • git diff --check

Checklist

  • Code has passed local testing
  • I have added tests that prove my fix is effective or that my feature works

@codecov-commenter

codecov-commenter commented Jun 9, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 53.50%. Comparing base (60d1c2a) to head (2f0c1f5).
⚠️ Report is 834 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #3387      +/-   ##
===========================================
+ Coverage    46.76%   53.50%   +6.73%     
===========================================
  Files          295      493     +198     
  Lines        17172    38350   +21178     
===========================================
+ Hits          8031    20520   +12489     
- Misses        8287    16194    +7907     
- Partials       854     1636     +782     

☔ 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.

@leno23

leno23 commented Jun 9, 2026

Copy link
Copy Markdown
Contributor Author

Addressed the remaining Codecov patch coverage feedback by covering the delete-event skip path and the valid-client DoUnregister path. Local validation passes: go test -coverprofile=/tmp/etcdv3.cover ./registry/etcdv3, go test ./registry/etcdv3 ./remoting/etcdv3, and git diff --check.

Comment thread registry/etcdv3/listener.go

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fills several missing lifecycle behaviors in the etcdv3 registry to bring it closer to other registries (notably zookeeper): unregistering removes the etcd key, subscribe/unsubscribe is managed per-service (rather than via a single shared listener), and subscriptions are preserved across client restarts by reconstructing listeners during InitListeners() recovery.

Changes:

  • Implement DoUnregister() by validating the etcd client and deleting the registered key.
  • Refactor subscription handling to per-service listeners, adding DoUnsubscribe() that closes/removes the service listener.
  • Add subscription recovery on reconnect by rebuilding listeners from previously subscribed URLs and restarting service-event watches; expand tests accordingly.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.

File Description
registry/etcdv3/registry.go Implements unregister/unsubscribe, switches to per-service subscription listeners, and adds listener recovery during re-init.
registry/etcdv3/listener.go Reworks the data listener to dispatch to multiple subscribed listeners and adds listener close semantics/state.
registry/etcdv3/registry_test.go Adds coverage for unregister, subscribe/unsubscribe behavior, listener close, and recovery logic.
registry/etcdv3/listener_test.go Adds coverage for dispatch behavior, wildcard matching, unsubscribe/close behavior, and listener close/next behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread registry/etcdv3/registry.go
Comment thread registry/etcdv3/listener.go
Comment thread registry/etcdv3/listener.go
Comment thread registry/etcdv3/registry.go
Comment thread registry/etcdv3/registry.go
Comment thread registry/etcdv3/registry.go Outdated
Comment thread registry/etcdv3/listener.go
@leno23

leno23 commented Jun 13, 2026

Copy link
Copy Markdown
Contributor Author

Follow-up for the remaining Codecov partial coverage warning in registry/etcdv3/listener.go: added a focused test that drives configurationListener.Next through the skipped delete-event path when the etcd client is invalid, then verifies the next add event is returned. Local validation passes: go test ./registry/etcdv3 ./remoting/etcdv3, go test -race ./registry/etcdv3, go test -coverprofile=/tmp/etcdv3_after.out ./registry/etcdv3, and git diff --check.

Comment thread registry/etcdv3/registry.go Outdated
@leno23

leno23 commented Jun 13, 2026

Copy link
Copy Markdown
Contributor Author

Addressed the P1 DoUnsubscribe feedback. DoUnsubscribe now validates the event listener and the subscribed entry type before calling unsubscribeURLLocked, so error paths no longer close/remove the local subscription. I also added a regression test covering the nil event-listener path and verifying the subscription remains in the map and open. Local validation passes: go test ./registry/etcdv3 ./remoting/etcdv3, go test -race ./registry/etcdv3, go test -coverprofile=/tmp/etcdv3_fix.out ./registry/etcdv3, and git diff --check.

@leno23

leno23 commented Jun 13, 2026

Copy link
Copy Markdown
Contributor Author

Follow-up for the failed CI lint job: changed the new error assertion to require.ErrorContains to satisfy testifylint. Local validation passes: go test ./registry/etcdv3 ./remoting/etcdv3, go test -race ./registry/etcdv3, and git diff --check. golangci-lint is not installed in this local environment, so I verified the exact reported lint issue by matching the CI log.

@sonarqubecloud

Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[FEATURE] etcdv3 registry: implement DoUnregister, DoUnsubscribe and reconnect subscription recovery

5 participants