RexXiong commented on PR #3858: URL: https://github.com/apache/celeborn/pull/3858#issuecomment-6525417308
Thanks for addressing the disk-capacity, disk-health, and configuration-prefix feedback. I have two remaining concerns: **1. [P2] Invalidate the local stats cache across HA leadership changes** `workerStatsStore.remove()` is called from the leader-only `Master.handleWorkerLost()` path. Consider this sequence: - Master A caches worker W’s stats while leading. - Leadership moves to B, with A remaining alive as a follower. - W is lost. B handles the event, while A applies it through `MetaHandler → updateWorkerLostMeta()`, which does not clear A’s local stats cache. - If A becomes leader again, W’s old stats remain. Since W is already absent from `workersMap`, the worker-timeout scan will not remove that entry. Could we invalidate the cache on leadership changes, or reconcile it with registered workers? Metrics can remain outside Raft state. Please also add an HA regression test covering this sequence. **2. Add an explicit collection switch and clarify configuration names** The collector currently starts unconditionally, and this path is not gated by `celeborn.metrics.enabled`. Since scaling policies are follow-up work, could we add a dedicated `celeborn.metrics.worker.scale.enabled` switch, preferably defaulting to `false` until the feature is explicitly enabled? This would also help address the earlier heartbeat-overhead concern. The new prefix looks reasonable. For the suffixes, I suggest: ```properties celeborn.metrics.worker.scale.collector.class celeborn.metrics.worker.scale.collect.interval celeborn.metrics.worker.scale.memory.slidingWindow.size ``` `collect.interval` distinguishes sampling from heartbeat reporting. More importantly, the sliding window currently applies only to memory usage, whereas `metricWindowSize` and its documentation suggest it applies to all collected metrics. Please document that scope and the current behavior of reporting zero until the window fills. The exact naming is a suggestion; clarifying the semantics is the main concern. *AI-assisted review by OpenAI Codex (GPT-6).* -- 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]
