Duansg commented on PR #4289:
URL: https://github.com/apache/hertzbeat/pull/4289#issuecomment-5162750729
Thanks for this — the problems you're fixing are real, and I confirmed all
three against master:
- MetricsFlushTask.run() drains a batch out of the queue and calls
doSaveData(), which swallows failures internally, so a failed batch is silently
lost;
- the overflow path calls doSaveData(contentList) — the whole list, once per
item that failed to be offered — producing duplicate writes;
- the reschedule sits inside the try, so an exception escaping the flush
kills the periodic chain permanently.
All worth fixing. But I think the new backpressure model is a blocking
regression.
Unbounded backpressure freezes the persistence pipeline
sendVictoriaMetrics() now loops with no exit condition other than success or
shutdown:
while (!offered) {
synchronized (metricsFlushLock) { offered =
metricsBufferQueue.offer(content); }
if (offered) break;
triggerImmediateFlush();
Thread.sleep(MAX_WAIT_MS); // no bound, no attempt limit
}
saveData() runs on the single warehouse-persistent-data-storage thread
(DataStorageDispatch.java:74-91), which also drives:
- calculateMonitorStatus() (line 86) — monitor up/down state
- pluginRunner.pluginExecute(PostCollectPlugin.class, ...) (line 88)
- realTimeDataWriter.saveData() (line 90) — real-time store backing the
dashboard
If VictoriaMetrics is rejecting writes and the buffer fills, that thread
blocks indefinitely and all four stop. metricsDataToStorageQueue is an
unbounded LinkedBlockingQueue (InMemoryCommonDataQueue.java:58), so the backlog
grows until the heap is exhausted.
To be precise about the blast radius: threshold alerting is not immediately
affected — MetricsRealTimeAlertCalculator.java:132 consumes
pollMetricsDataToAlerter(), a separate queue and thread. But once the storage
queue exhausts the heap, that goes down with everything else.
The existing availability check does not save you here.
checkVictoriaMetricsDatasourceAvailable() probes vmselect
(VictoriaMetricsClusterDataStorage.java:147, vmClusterProps.select().url()),
while this PR is about vminsert write failures. With vminsert down and vmselect
healthy, serverAvailable stays true and execution falls straight into the loop
above.
Trading bounded data loss for an unbounded stall is a bad trade for a
monitoring system — a history-store outage should degrade history, not freeze
status calculation and real-time writes.
Suggestion: either keep a bounded retry (N attempts / a total time budget)
and then drop with a counter or metric, or move the VM write path onto its own
executor so backpressure cannot propagate back into the shared consumer thread.
The same defects remain in the single-node storage
VictoriaMetricsDataStorage has essentially the same code and is untouched by
this PR:
- sendVictoriaMetrics() at lines 572-612 still has the
doSaveData(contentList) overflow path that re-sends the entire batch;
- MetricsFlushTask.run() at lines 627-651 still calls
triggerIntervalFlushTimer() inside the try, so the chain can still die, and a
drained batch is still lost on failure.
Since the single-node deployment is the more common one, fixing only the
cluster variant leaves most users exposed. Worth either covering both here or
extracting the shared flush logic.
Minor
Replacing log.error("... failed. {}", responseEntity.getBody()) with
status-only logging removes the main diagnostic for write rejections (bad
label, out of disk, retention policy). Keeping the body at DEBUG would preserve
troubleshooting without the noise.
--
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]
