lymerin commented on PR #7289: URL: https://github.com/apache/shenyu/pull/7289#issuecomment-5855023499
> Thanks for taking on #6479 — stale discovery upstreams surviving a selector deletion is one of those bugs that is invisible in tests and painful in production, and this PR wires the whole chain end to end rather than patching one hop: > > * admin side resolves (and now requires) the plugin name before publishing a removal (`AbstractPathDataChangedListener`, `SelectorServiceImpl`), which is the root cause of the malformed `.../discoveryUpstream//<selectorId>` paths; > * gateway side finally acts on a removal, per protocol: divide/websocket drop their `UpstreamCacheManager` entry, gRPC invalidates `ApplicationConfigCache`/`GrpcClientCache`, and TCP gets the selector-id → selector-name mapping it needed (`UpstreamProvider#registerSelector` / `getSelectorName`) because the upstream cache there is keyed by _name_ while the sync key carries the _id_; > * the three sync channels are covered differently and correctly for each: ZK now uses `oldData` for `NODE_DELETED` when the payload is gone, the websocket handler maps `doDelete` to `DiscoveryUpstreamKey`, and the HTTP refresh diffs the previous snapshot so a selector that disappears from the config (or whose name changed) is properly removed. > > The `DiscoveryUpstreamKey` record is the right abstraction — it captures exactly what a removal needs (plugin + id + optional name) instead of smuggling a partially-filled `DiscoverySyncData` around, and it is why `CommonDiscoveryUpstreamDataSubscriber#unSubscribe` can finally be implemented instead of being an `//ignore`. > > ### Requesting changes for two things > **1. The branch has conflicts and zero CI evidence.** GitHub reports no workflow runs on `fix/6479-discovery-upstream-delete-cache` and the merge state is dirty. This patch spans seven modules (admin listener api, admin, plugin-base, divide, grpc, tcp, websocket, protocol-tcp, sync-data-api, http, websocket, zookeeper) and changes delete semantics in production paths, so please rebase onto current master and get a green `pr_build` before this is merged — I cannot approve it on reading alone. > > **2. `SelectorServiceImpl#unbindDiscovery` now throws `IllegalStateException` (SelectorServiceImpl.java:332).** The intent — "do not publish a removal we cannot address" — is right, and validating the whole batch before touching any row (the two-phase `ResolvedDiscovery` structure) is a nice touch. But the consequence is that a batch delete that contains **one** selector with unresolvable plugin metadata now fails and rolls back entirely, and the admin REST layer sees a raw `IllegalStateException` rather than a typed error. Given that this can happen from stale/orphan `discovery` rows, users could end up unable to delete selectors at all. Options: > > * degrade gracefully: skip the discovery unbind for that selector, log at ERROR, and continue deleting the rest of the batch (the row-level cleanup then still happens for the healthy ones), or > * keep failing, but throw a typed admin exception (the ones already mapped to a clean error response) instead of `IllegalStateException`. > > Either is fine for me; please pick one and make the behaviour visible in the message shown to operators. > > ### Worth noting, not blocking > * **Public API change.** `DiscoveryUpstreamDataSubscriber#unSubscribe(DiscoverySyncData)` became `unSubscribe(DiscoveryUpstreamKey)`. I checked every occurrence in the repo: `CommonDiscoveryUpstreamDataSubscriber` is the only implementation and every other call site goes through the sync services you already updated, so the build is fine — but this is source-incompatible for anyone implementing the interface downstream, so it belongs in the release notes. > * **Coverage of the other sync channels.** Discovery-upstream deletion is now propagated only over zookeeper, websocket and http. Nacos, etcd, consul, polaris and apollo still have no delete path for this group (pre-existing). A short note in the PR description or a follow-up issue would help whoever hits it next. > * The blank-`pluginName` guard in `AbstractPathDataChangedListener` is the right safety net; `selectorId` has the same failure shape (`buildDiscoveryUpstreamPath` would produce `.../divide/null`). No logging detail lost if you extend the same check to it. > * In `DiscoveryUpstreamDataRefresh#refresh`, an empty snapshot now triggers `subscriber.refresh()` **and** an `unSubscribe` per previously known key. Correct, just slightly redundant work; also note `previousSnapshot` being a plain `HashMap` is fine only because you made `refresh` `synchronized` — worth a comment so nobody removes the modifier later. > * `TcpUpstreamDataHandler` registering the selector id → name mapping inside `handlerDiscoveryUpstreamData` is the piece that makes `removeDiscoveryUpstreamData` work after a gateway restart with no prior selector event. Good; please keep the invariant documented on `UpstreamProvider#registerSelector`. > > Once the branch is rebased with green CI and the `IllegalStateException` handling is settled, I am happy to take another look. Addressed each point: 1. Resolved the conflicts against master; `pr_build` passed. 2. Kept batch deletion atomic, replaced `IllegalStateException` with `ShenyuAdminException`, and made the error state that no selectors in the batch were deleted. 3. Documented the `unSubscribe` API change in the release notes. 4. Clarified in the PR description that Nacos, etcd, Consul, Polaris, and Apollo **do have** discovery-upstream deletion paths through the shared handlers. 5. Added the blank `selectorId` guard. 6. Documented why `previousSnapshot` uses `HashMap` under synchronized `refresh`. 7. Retained TCP’s selector ID-to-name registration for deletion after a gateway restart. -- 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]
