hengyuss commented on PR #7289:
URL: https://github.com/apache/shenyu/pull/7289#issuecomment-5883457659
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:
```text
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 #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 #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?
--
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]
