lymerin commented on PR #7289: URL: https://github.com/apache/shenyu/pull/7289#issuecomment-5885571487
> Thanks for working on this issue. > > After comparing this PR with #7172, I do not think the new `DISCOVER_UPSTREAM DELETE` chain is the correct way to remove discovery upstreams. > > PR #7172 establishes discovery upstream synchronization as snapshot reconciliation: > > ``` > Registry ADDED / UPDATED / DELETED > -> Admin updates the database > -> Admin queries the complete remaining upstream list > -> Publish DISCOVER_UPSTREAM UPDATE > -> Gateway reconciles its local cache with the snapshot > ``` > > This means a registry DELETED event is an internal Admin-side event. It should not be propagated to the gateway as an upstream DELETE event. > > For example: > > Previous upstreams: [A, B] Delete A Published snapshot: [B] > > The gateway removes A by comparing [A, B] with [B]. > > Deleting the last instance should still publish an empty UPDATE snapshot: > > Previous upstreams: [A] Delete A Published snapshot: [] > > The gateway then clears the selector's upstream cache through the same reconciliation path. > > Therefore, the following chain introduced by this PR represents the wrong abstraction for upstream instance deletion: > > DISCOVER_UPSTREAM DELETE -> DiscoveryUpstreamDataSubscriber.unSubscribe() -> DiscoveryUpstreamDataHandler.removeDiscoveryUpstreamData() > > There are two different concepts that should not be mixed: > > 1. An upstream instance is removed > This should be handled by a complete DISCOVER_UPSTREAM UPDATE snapshot, as implemented by [ fix: reconcile upstream snapshots without retaining removed instances #7172](https://github.com/apache/shenyu/pull/7172). > 2. An entire selector is removed > Incremental synchronization already publishes SELECTOR DELETE, which invokes the plugin's removeSelector() method. > Divide, WebSocket, and gRPC already clean their plugin-specific caches through this path. > > The actual remaining issue is full-snapshot reconciliation, especially for HTTP sync: > > Previous selectors: [S1, S2] Current selectors: [S2] > > SelectorDataRefresh does not invoke removeSelector(S1), and DiscoveryUpstreamDataRefresh does not detect that S1 disappeared. This can leave the deleted selector's upstream cache behind. > > That problem should be fixed by reconciling the previous and current snapshots, rather than introducing a new discovery-upstream DELETE lifecycle. > > I suggest: > > * Keep upstream instance removal based on the complete UPDATE snapshot from [ fix: reconcile upstream snapshots without retaining removed instances #7172](https://github.com/apache/shenyu/pull/7172). > * Do not introduce removeDiscoveryUpstreamData() for ordinary upstream removal. > * Do not replace unSubscribe(DiscoverySyncData) with the incompatible unSubscribe(DiscoveryUpstreamKey) API. > * Fix HTTP synchronization by calculating: > removed = previousSnapshot - currentSnapshot > * For removed selectors, invoke the existing selector/plugin cleanup path, or introduce a compatible snapshot > reconciliation API. > * Add regression tests for: > S1: [A, B] -> S1: [B] > S1: [A] -> S1: [] > [S1, S2] -> [S2] > [S1] -> [] > * Handle WebSocket MYSELF/REFRESH reconciliation separately if reconnect recovery is also in scope. > > In summary, the stale-cache problem is valid, but the DELETE chain introduced by this PR conflicts with the snapshot- based upstream synchronization model established by #7172. I suggest redesigning this PR around snapshot reconciliation before merging. @lymerin what do you think? I agree with the instance-level model in #7172: registry `ADDED`, `UPDATED`, and `DELETED` events should publish `DISCOVER_UPSTREAM UPDATE` with the remaining upstream list, including an empty list. #7289 does not change that producer or turn an instance deletion into `DELETE`. The `DISCOVER_UPSTREAM DELETE` handled here is an existing **selector-level** event. `SelectorServiceImpl#unbindDiscovery` already calls `removeSelectorUpstream`, which publishes it; #7289 fixes the gateway path that ignored the event or reached a no-op `unSubscribe`—the failure reported in #6479. Existing `SELECTOR DELETE` and `PROXY_SELECTOR DELETE` handling remain unchanged. The proposed HTTP `previous − current` reconciliation is also in this PR. Its tests cover `[S1, S2] → [S2]` and `[S1] → []`. An existing selector with an empty upstream list stays in the Admin snapshot, so it is not treated as a removed selector. I’ve clarified these separate flows in the PR description. The `unSubscribe` signature change is documented in the release notes; the instance-level `[A, B] → [B]` and `[A] → []` cases belong to #7172. > Thanks for working on this issue. > > After comparing this PR with #7172, I do not think the new `DISCOVER_UPSTREAM DELETE` chain is the correct way to remove discovery upstreams. > > PR #7172 establishes discovery upstream synchronization as snapshot reconciliation: > > ``` > Registry ADDED / UPDATED / DELETED > -> Admin updates the database > -> Admin queries the complete remaining upstream list > -> Publish DISCOVER_UPSTREAM UPDATE > -> Gateway reconciles its local cache with the snapshot > ``` > > This means a registry DELETED event is an internal Admin-side event. It should not be propagated to the gateway as an upstream DELETE event. > > For example: > > Previous upstreams: [A, B] Delete A Published snapshot: [B] > > The gateway removes A by comparing [A, B] with [B]. > > Deleting the last instance should still publish an empty UPDATE snapshot: > > Previous upstreams: [A] Delete A Published snapshot: [] > > The gateway then clears the selector's upstream cache through the same reconciliation path. > > Therefore, the following chain introduced by this PR represents the wrong abstraction for upstream instance deletion: > > DISCOVER_UPSTREAM DELETE -> DiscoveryUpstreamDataSubscriber.unSubscribe() -> DiscoveryUpstreamDataHandler.removeDiscoveryUpstreamData() > > There are two different concepts that should not be mixed: > > 1. An upstream instance is removed > This should be handled by a complete DISCOVER_UPSTREAM UPDATE snapshot, as implemented by [ fix: reconcile upstream snapshots without retaining removed instances #7172](https://github.com/apache/shenyu/pull/7172). > 2. An entire selector is removed > Incremental synchronization already publishes SELECTOR DELETE, which invokes the plugin's removeSelector() method. > Divide, WebSocket, and gRPC already clean their plugin-specific caches through this path. > > The actual remaining issue is full-snapshot reconciliation, especially for HTTP sync: > > Previous selectors: [S1, S2] Current selectors: [S2] > > SelectorDataRefresh does not invoke removeSelector(S1), and DiscoveryUpstreamDataRefresh does not detect that S1 disappeared. This can leave the deleted selector's upstream cache behind. > > That problem should be fixed by reconciling the previous and current snapshots, rather than introducing a new discovery-upstream DELETE lifecycle. > > I suggest: > > * Keep upstream instance removal based on the complete UPDATE snapshot from [ fix: reconcile upstream snapshots without retaining removed instances #7172](https://github.com/apache/shenyu/pull/7172). > * Do not introduce removeDiscoveryUpstreamData() for ordinary upstream removal. > * Do not replace unSubscribe(DiscoverySyncData) with the incompatible unSubscribe(DiscoveryUpstreamKey) API. > * Fix HTTP synchronization by calculating: > removed = previousSnapshot - currentSnapshot > * For removed selectors, invoke the existing selector/plugin cleanup path, or introduce a compatible snapshot > reconciliation API. > * Add regression tests for: > S1: [A, B] -> S1: [B] > S1: [A] -> S1: [] > [S1, S2] -> [S2] > [S1] -> [] > * Handle WebSocket MYSELF/REFRESH reconciliation separately if reconnect recovery is also in scope. > > In summary, the stale-cache problem is valid, but the DELETE chain introduced by this PR conflicts with the snapshot- based upstream synchronization model established by #7172. I suggest redesigning this PR around snapshot reconciliation before merging. @lymerin what do you think? PR #7172 resolves instance lifecycle within a selector, while PR #7289 resolves selector lifecycle within discovery sync. I compared #7289 with #7172 and ran the relevant tests on both branches. I agree with the snapshot model for **upstream instance** changes, but the proposed redesign is based on a misunderstanding of which event #7289 handles. 1. **Registry instance deletion remains an UPDATE snapshot.** A registry `DELETED` event is handled inside Admin. `DiscoveryDataChangedEventSyncListener` publishes `DISCOVER_UPSTREAM UPDATE` containing the complete remaining upstream list, including an empty list after the last instance is deleted. #7289 does not change this publisher or turn that event into `DISCOVER_UPSTREAM DELETE`. The tests on #7172 verify that deleting A from `[A, B]` publishes `[B]` and leaves only B in the gateway cache; deleting A from `[A]` publishes `[]` and clears the cached instance. The incorrect result you describe—deleting the entire selector when only A is removed—does not occur through #7289’s DELETE handler. 2. **The discovery-upstream DELETE event is selector-scoped and predates this PR.** Admin’s `removeSelectorUpstream()` already published `DISCOVER_UPSTREAM DELETE` when unbinding a selector-level discovery. Before #7289, `CommonDiscoveryUpstreamDataSubscriber#unSubscribe()` ignored that event. This PR makes that existing selector-removal event effective; it does not introduce a registry-instance DELETE lifecycle. I agree that `SELECTOR DELETE` already cleans some plugin caches, including the Divide, WebSocket, and gRPC paths you identified. That does not make the previously published but ignored discovery-upstream removal event an instance-deletion event. 3. **The HTTP omission you identified is fixed by snapshot comparison.** `DiscoveryUpstreamDataRefresh` now compares the previous and current selector snapshots. In the #7289 tests, `[S1, S2] → [S2]` unsubscribes S1 but not S2, while `[S1] → []` unsubscribes S1 once. A separate plugin-handler test starts with an upstream entry and verifies that removing its discovery selector leaves no Divide upstream cache entry. Thus the HTTP fix is snapshot reconciliation for *missing selectors*; it does not replace #7172’s UPDATE reconciliation for *missing instances*. 4. **`removeDiscoveryUpstreamData()` is not used for ordinary instance removal.** Instance changes continue through `onSubscribe()` with the complete UPDATE snapshot. The removal method is invoked for the selector identity carried by an existing discovery-upstream DELETE event, or for a selector missing from an HTTP snapshot. For that reason, I am not replacing this path with instance-level snapshot logic. 5. **I am retaining `DiscoveryUpstreamKey` intentionally.** This removal operation needs the selector’s identity, not a partially populated `DiscoverySyncData`. All implementations and call sites in this repository have been migrated. I acknowledge the source- and binary-compatibility impact for downstream implementations; it is explicitly documented in `RELEASE-NOTES.md`. A compatibility bridge back to `DiscoverySyncData` would recreate the partial DTO that this change removes. 6. **The four requested regression scenarios are covered by the two PRs according to their respective responsibilities.** #7172 tests instance snapshots `[A, B] → [B]` and `[A] → []`, including the resulting cache state. #7289 tests HTTP selector snapshots `[S1, S2] → [S2]` and `[S1] → []`, and separately tests removal from a plugin upstream cache. I ran `DiscoveryDataChangedEventSyncListenerTest` and `UpstreamCacheManagerTest` on #7172, and `DiscoveryUpstreamDataRefreshTest` and `DivideUpstreamDataHandlerTest` on #7289; the assertions described above all passed. 7. **WebSocket MYSELF/REFRESH reconnection reconciliation is separate.** #7289 handles its DELETE event and retains the existing `refresh()` hook, but does not claim to implement a new reconciliation algorithm for reconnection. I am not expanding this PR to cover that case. So I agree with your requirement that `[A, B] → [B]` and `[A] → []` use the full UPDATE snapshot from #7172. They still do. #7289 addresses the different case of a discovery-bound selector being removed, plus selectors disappearing from an HTTP snapshot. The two PRs are complementary rather than conflicting, so I do not think #7289 needs to be redesigned around instance-level snapshot reconciliation. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
