Kalvin2077 commented on PR #3858: URL: https://github.com/apache/celeborn/pull/3858#issuecomment-6569005390
> 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: > > ```ini > 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)._ @RexXiong Thx for your reviews. Cache cleanup now runs in the metadata apply path, covering followers and snapshot restoration; the three-node A → B → A regression passes. Added `celeborn.metrics.worker.scale.enabled`, defaulting to `false` and independent of `celeborn.metrics.enabled`. When disabled, the collector is not constructed or started. I adopted `collector.class`, `collect.interval`, and `memory.slidingWindow.size`. The documentation distinguishes sampling from heartbeat reporting, states that the window only averages memory usage, and explains the zero value before it fills. Tests cover default-disabled collection and explicitly enabled collection with general metrics disabled. -- 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]
