Aias00 commented on PR #6528: URL: https://github.com/apache/shenyu/pull/6528#issuecomment-5156775583
Reviewed #6528 — fix(admin): persist upstream status changes into selector handle. Verified the fix and the four new tests — they are real regression tests (they fail without `syncUpstreamStatus` because `existList[0].status` stays `true`). The root-cause analysis is correct: `CommonUpstream.equals()` (omits `status`) means a status-changed upstream still matches its old entry in `existList`, so the old status was serialized back into the handle. The new helper writes the new status onto the matching entry before serialization, which is the right minimal fix. The symmetric `false -> true` (re-register) path also works via the same predicate. CI is green. One scope question worth a conscious maintainer call: - **`ShenyuClientRegisterWebSocketServiceImpl.buildHandle` (lines 78-96) has the same — actually worse — bug and is not touched by this PR.** It never computes `diffStatusList` at all, only `diffList` for new upstreams, so a DELETED/offline status change is lost from the WebSocket selector handle exactly as in #6522. The new `syncUpstreamStatus` helper alone wouldn't fix it (it needs the `diffStatusList` detection block first), so it's a slightly larger change. Suggest either folding it in here for completeness or opening a follow-up issue so it isn't lost. Minor nits (not blocking): - The PR body claims the "going back online" symmetric path works, but no test covers `status=false -> true`. One extra test (seed the handle with `status=false`, re-register, assert `true`) would close the loop. - Pre-existing: `isEventDeleted = uriList.size() == 1 && EventType.DELETED` in Dubbo/Grpc/Tars means batch DELETED events don't get `status=false` set, so `syncUpstreamStatus` won't flag them either. Divide is unaffected (per-upstream `eventType` in the builder). Worth a separate follow-up. -- 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]
