CAICAIIs commented on PR #1138: URL: https://github.com/apache/incubator-seata-go/pull/1138#issuecomment-5122414488
> # 中文 / Chinese > 高质量的并发设计,GSOC 这一版基础打得很扎实。几个关键点我都过了一遍,没有发现 race 或泄漏: > > * **initial-before-changes 顺序成立**:`subscribeWithSnapshot` 在 store 锁内同时"注册回调 + 取快照",所以 initial 反映注册时刻状态,之后的 Update 才走回调 publish;`nextEvent()` 永远先吐 `initial` 再吐 `latest`,因果顺序正确。 > * **coalescing 安全**:全量快照语义下,慢监听者跳过中间快照没问题;`wakeCh`(buffered 1)+ default 丢弃 + 每轮 drain `latest`,不会丢最终态。 > * **无 goroutine 泄漏**:`doneCh` 关闭让 dispatch 退出;registry Close 时遍历 Unsubscribe 所有订阅;`start()` 在 closed 时早退、Unsubscribe 用 `once` 防重复 close。 > * **etcd watch revision 修复是真实收获**:`WithRev(resp.Header.Revision+1)` 从快照 revision 续 watch,消除了 Get/Watch 之间的事件丢失窗口,且有测试覆盖。 > * 单个 `etcdClusterPrefix` 前缀 watch 在构造时启动、喂所有 cluster 的 store,所以 `Subscribe` 不需要自己起 watch —— 这点设计正确,changes 一定会送达。 > > 几个小问题(非阻塞): > > ### 1. 拼写:`"cluster doesnt exit"` > `Lookup` 和 `Subscribe` 两处都是 `"cluster doesnt exit"`,应为 `"cluster doesn't exist"`。 > > ### 2. `vgroupMapping` 的并发读 > `Lookup` / `Subscribe` 里 `s.vgroupMapping[key]` 是无锁 map 读。如果它在构造后不可变,没问题;但请确认没有任何路径会在运行时写它(否则是 data race)。加一行注释说明"init 后只读"会更稳。 > > ### 3. Subscribe 里注册回调早于 closed 检查的窗口 > `subscribeWithSnapshot` 先把回调注册进 store,之后才 `Lock` 检查 `s.closed`;若已 closed 则 Unsubscribe 清理。窗口很短且会被清理,可接受,但值得一句注释点明这个顺序是有意为之。 > > ## 小结 > 设计和实现都很干净,approve 方向。上面都是 nice-to-have,#1 拼写建议顺手改掉。非目标(Nacos/ZK/Consul/Redis 适配、canceled/compacted watch 恢复)明确列出,范围克制,很好。 > > # English > High-quality concurrency design — a solid GSOC foundation. I walked through the key invariants and found no races or leaks: > > * **initial-before-changes ordering holds**: `subscribeWithSnapshot` registers the callback and takes the snapshot atomically under the store lock, so `initial` reflects state at registration time and only later Updates go through the callback's publish; `nextEvent()` always returns `initial` before `latest`, preserving causal order. > * **Coalescing is safe**: with full-snapshot semantics, letting slow listeners skip intermediate snapshots is fine; `wakeCh` (buffered 1) + default-drop + draining `latest` each round never loses the final state. > * **No goroutine leak**: closing `doneCh` exits dispatch; registry Close iterates and Unsubscribes all subscriptions; `start()` early-returns when closed and Unsubscribe uses `once` to avoid a double close. > * **The etcd watch-revision fix is a real win**: `WithRev(resp.Header.Revision+1)` resumes the watch from the snapshot revision, closing the event-loss window between Get and Watch, and it's covered by a test. > * A single `etcdClusterPrefix` prefix-watch starts at construction and feeds the store for all clusters, so `Subscribe` doesn't need to start its own watch — correct, and changes are guaranteed to be delivered. > > A few minor points (non-blocking): > > ### 1. Typo: `"cluster doesnt exit"` > Both `Lookup` and `Subscribe` use `"cluster doesnt exit"` — should be `"cluster doesn't exist"`. > > ### 2. Concurrent read of `vgroupMapping` > `s.vgroupMapping[key]` in `Lookup` / `Subscribe` is a lock-free map read. If it's immutable after construction, fine; please confirm nothing writes it at runtime (otherwise it's a data race). A "read-only after init" comment would help. > > ### 3. Window where the callback is registered before the closed check in Subscribe > `subscribeWithSnapshot` registers the callback into the store before the `Lock`/`s.closed` check; if closed, it Unsubscribes to clean up. The window is short and cleaned up, so it's acceptable, but a comment noting the ordering is intentional would help. > > ## Summary > Clean design and implementation — approve direction. The above are nice-to-haves; the #1 typo is worth a quick fix. Non-goals (Nacos/ZK/Consul/Redis adapters, recovery from canceled/compacted watches) are clearly listed and scope is well-contained. Nice work. Thanks for the detailed review! -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
