Duansg commented on PR #4196: URL: https://github.com/apache/hertzbeat/pull/4196#issuecomment-5015399955
> If we make this change, could it result in an unmanageable number of tasks? When a task is deemed unavailable, is there no longer a need to retry its other subtasks? @tomsun28 @hengyuss That's right; simply removing the `priority == 0` check isn't safe in and of itself, but I still think we need to remove this validation. Regarding the background of this PR, here's my take: 1. Would tasks grow unmanageably? - No task repetition: The timeout dispatch only advances the task’s `priorMetrics` state machine; the next-level metric it triggers is precisely what should be executed in that round. `cyclicJob` only restarts after the current round is complete, so rounds are never repeated. Additionally, since #4169/#4201, the timer round mechanism has been fully safeguarded. - Issue 1: Currently, there is no mechanism to cancel a worker that remains suspended (e.g., due to I/O blocking). Once this check (for `priority == 0`) is removed, the catch-all thread will continue to reschedule the worker in every round; therefore, a metric that never returns will leak a permanently stuck worker task in every round until the thread pool is exhausted; - Issue 2: This change will exacerbate an existing race condition. For example, a task that eventually completes after timing out (>4 min) will still trigger a second call to `dispatchCollectData` 2. When a task is deemed unavailable, do we still skip its other metrics? - The current logic is that if an availability collection with priority 0 fails or times out, `isAvailableCollectFailed` will still cause the entire round to terminate early, and none of the tasks below it will be scheduled. The only change in behavior occurs when the availability collection succeeds but a non-availability metric is suspended: currently, this causes the monitor to become permanently stalled, because `cyclicJob` is only restarted within `dispatchCollectData`, and the system does not schedule tasks for suspended metrics. - Assumption: Suppose the task does indeed resume after 30 minutes. During those 30 minutes, there is no data, no TIMEOUT events, and no alerts; the page displays what appears to be normal, but outdated data. “Waiting for it to recover on its own” effectively leaves the decision on when to detect the delay up to the party responsible for the failure. While this may not matter to the data collector, it is disastrous for those using the monitoring system. -- 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]
