Duansg commented on PR #4289:
URL: https://github.com/apache/hertzbeat/pull/4289#issuecomment-5243120994
Two follow-ups: the flush lock doesn't cover the HTTP write, and dropped
metrics have no signal outside the log
Both apply to VictoriaMetricsClusterDataStorage and
VictoriaMetricsDataStorage — the code is parallel in the two files.
Replacing the unbounded while (!offered) retry with a bounded offer is the
right call, and pulling the standalone storage in alongside the cluster one
closes the gap the earlier revision left. Two things I'd like to see addressed
before this lands.
---
1. retryBatch is not claimed while it is being sent
flushBufferedMetrics() releases metricsFlushLock before the POST, and leaves
retryBatch pointing at the batch for the whole duration of the write:
```java
List<VictoriaMetricsContent> batch;
synchronized (metricsFlushLock) {
if (retryBatch.isEmpty()) {
List<VictoriaMetricsContent> nextBatch = new ArrayList<>(...);
metricsBufferQueue.drainTo(nextBatch, ...);
retryBatch = nextBatch;
}
batch = retryBatch;
} // lock released here
if (batch.isEmpty()) return true;
if (!trySaveData(batch)) { ... return false; } // HTTP POST, unsynchronised
synchronized (metricsFlushLock) {
if (retryBatch == batch) retryBatch = Collections.emptyList();
}
```
There is no in-flight marker. A second caller entering this method during
the POST sees retryBatch non-empty, takes the same list reference, and sends it
again. The success path clears by reference identity, so the loser of the race
simply doesn't clear — the samples have already gone twice. The critical
section guards the drain and the clear, but not the state that actually matters.
This is latent rather than live today, and I want to be precise about why,
because the reason isn't in this file:
- MetricsFlushTask and ImmediateMetricsFlushTask are both scheduled on the
single metricsFlushTimer, and HashedWheelTimer invokes task.run(this) inline on
its one worker thread (HashedWheelTimer.java:645, reached from expire() at line
722). They serialise on that thread.
- destroy() runs on the shutdown thread, but calls metricsFlushTimer.stop()
before its drain loop, and stop() (lines 350-359) repeats
workerThread.interrupt(); workerThread.join(100); until the worker is dead. So
the shutdown flush cannot overlap an in-flight timer flush either.
So the mutual exclusion protecting retryBatch during the write comes from
the timer's single-threaded execution model plus stop()'s join — not from
metricsFlushLock. Nothing in flushBufferedMetrics() states or enforces that
dependency, and it stops holding as soon as anyone supplies the timer with a
task executor, adds a second flush trigger, or calls doSaveData — now public
and delegating straight to trySaveData — from another thread.
Suggested fix, either is cheap:
- Claim the batch under the lock: set retryBatch = Collections.emptyList()
while holding it, keep the reference locally, and restore it under the lock on
failure; or
- Add an AtomicBoolean flushInFlight and return early when a flush is
already running, in the same spirit as the existing immediateFlushPending.
Unrelated but worth noting alongside it: the retry path is at-least-once by
construction — a POST that times out after VictoriaMetrics accepted the batch
will be resent on the next flush. That's inherent to retrying and separate from
the concurrency point above; I mention it only so the two aren't conflated when
someone looks at duplicate samples.
---
2. Dropped metrics are effectively invisible to operators
After MAX_BUFFER_OFFER_ATTEMPTS (3) offers at MAX_WAIT_MS (500ms) each —
roughly 1.5s — the producer gives up on the rest of the batch:
```java
if (!offered) {
recordDroppedMetrics(contentList.size() - index, "buffer remained full");
return;
}
```
That discards every remaining item from index onward, permanently. Bounded
loss instead of blocking the warehouse-persistent-data-storage thread is the
right trade — that thread also drives calculateMonitorStatus(),
realTimeDataWriter.saveData() and PostCollectPlugin, so blocking it was worse.
But the loss needs to be visible, and right now it isn't:
- getDroppedMetricCount() is package-private and isn't wired into any
metric, health indicator, or UI surface — nothing outside the tests reads it;
- the log itself is throttled to the first occurrence, powers of two, and
multiples of 100 (total == count || (total & (total - 1)) == 0 || total % 100
== 0), so sustained loss produces sparse lines rather than steady evidence.
An operator whose VM insert endpoint is degraded will silently lose history
with no indication in the product. Exposing the counter — as an
actuator/Micrometer gauge, or through the existing storage-status endpoint that
the monitor pages already poll — would make the trade-off honest.
--
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]
