Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

• Queue this pull request

Up to standards ✅

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.}

Review: batch time-series HTTP ingest

Solid, well-motivated change. It reuses the existing TimeSeriesEngine.appendBatch (previously test-only) instead of inventing new machinery, and the per-measurement grouping with LinkedHashMap/LinkedHashSet correctly preserves the first-occurrence drop-set ordering the client-facing 400 relies on. The PR description, thread-dump evidence, and before/after table are excellent. Both handlers call appendBatch inside an active database.begin(), so appendBatch takes its in-thread sequential per-shard path - the HA-safe one that avoids publishing shard pages out of band with the enclosing commit. Good.

A few points, none blocking:

1. Prometheus handler: hoist tag/label values out of the per-sample loop (perf). In PostPrometheusWriteHandler.java (~L120-133), the labels of a remote-write TimeSeries are per-series, not per-sample, yet findLabelValue(ts.getLabels(), col.getName()) - a linear scan of the label list - is now recomputed for every sample x every TAG column, always returning the same value. Before batching this ran once (one sample); now it runs count times per tag with identical results. Since the value grid is column-major, you can resolve each TAG column value once before the sample loop and just fill the row. Given the project performance mantra this is worth doing. It does NOT apply to the line-protocol handler, where sample.getTags().get(...) genuinely varies per line - keeping that per-sample is correct.

2. Prometheus batching has no regression test asserting the contract (test coverage). The new PostTimeSeriesWriteHandlerBatchAppendIT is a nice deterministic, timing-free assertion via the writeTx statistic - but it only covers the line-protocol handler. The PostPrometheusWriteHandler change is exercised only for correctness by the existing PrometheusRemoteWriteReadIT, which does not assert the batching (transaction-count) contract. A regression there could silently revert Prometheus to per-sample commits without any test failing. Consider an analogous writeTx-delta assertion for a multi-sample remote-write series.

3. Empty remote-write series still auto-creates a type (minor, pre-existing). PostPrometheusWriteHandler calls getOrCreateType(...) before the (count == 0) continue guard, so a TimeSeries carrying labels but zero samples still creates a schema type with no data. This matches pre-change behavior (the old sample loop simply did not execute), so it is not a regression - but if you are touching this path, moving the empty check ahead of getOrCreateType would avoid the surprise.

4. Partial-write window on multi-measurement failure (pre-existing, worth documenting). TimeSeriesShard.appendSamples runs its own begin/commit on getWrappedDatabaseInstance(), so each appendBatch commits at the shard level immediately rather than deferring to the handler outer commit(). If a later measurement appendBatch throws, the handler database.rollback() cannot undo earlier measurements already-committed shard writes, and the client gets a 500 with partial data persisted. This predates the PR (per-sample appendSamples committed independently too), so it is not introduced here - but batching reshapes the granularity (per-measurement rather than per-sample); a line in the code/docs would keep the outer begin/commit from being mistaken for atomicity across measurements.

Minor. Schema lookups (existsType/getType) in PostTimeSeriesWriteHandler now run before database.begin() rather than inside it. Fine for reads, just noting the move.

Overall the correctness reasoning holds and the fix targets a real, well-evidenced HA hot spot. Addressing (1) and (2) would round it out.

Addressed in 95f212b. Notes per item.

1. Prometheus tag hoisting — done, but the premise needs a correction.

The optimization is right and is now implemented. The stated reason is not: this is not extra work introduced by batching. The pre-change code also looped for (final Sample sample : ts.getSamples()), so findLabelValue already ran samples × tagColumns times per series. The batching commit moved that work around but did not add any.

What is true, and what makes it worth fixing, is that the value is invariant across samples and the column-major grid makes it trivial to exploit — plus findLabelValue is worse than a plain linear scan: it calls sanitizeColumnName on every label it visits, which is 3 String.replace allocations per label. So the cost was samples × tagColumns × labels × 3 allocations, in both the old and new shapes.

Filling the grid column by column instead of sample by sample lets each TAG column resolve once and broadcast down its column via Arrays.fill. Agreed that the line-protocol handler must stay per-sample — sample.getTags() genuinely varies per line — and it is unchanged.

2. Prometheus batching regression test — done, and it needed the shard count.

PostTimeSeriesWriteHandlerBatchAppendIT is renamed to TimeSeriesIngestBatchAppendIT and now owns the batching contract for both ingest handlers, with two remote_write cases (single series; two series of the same metric differing only by tag).

Writing it surfaced something worth flagging: my first attempt reused the line-protocol tests' hardcoded ≤ 2 bound and failed at 11. That is not a batching failure — an auto-created remote-write type gets ASYNC_WORKER_THREADS shards (not 1, unlike the line-protocol tests which pin SHARDS 1 in DDL), and appendBatch opens at most one transaction per shard that received samples. 11 shards, 11 transactions, exactly the contract. A hardcoded bound there would have been machine-dependent and flaky across CI runners with different CPU counts, so the assertion now reads the shard count back off the type and binds to shards + 1 (and asserts shards < SAMPLES / 4 so the bound stays meaningful).

Verified the guard actually discriminates rather than merely passing: with the per-sample shape temporarily restored, both new cases fail at 200 transactions against a bound of 12.

3. Empty remote-write series auto-creating a type — not changing this.

Correctly identified as pre-existing and not a regression; getOrCreateType ran before the (absent) sample loop too, so behaviour is byte-for-byte identical. I am leaving it: moving the empty check ahead of getOrCreateType is a semantic change to schema auto-creation on a path this PR only touches for batching, and a zero-sample series carrying labels is a plausible way for a client to pre-register a metric. That is a decision for someone with the Prometheus-compat context, not a drive-by in a perf PR. Happy to file it separately if wanted.

4. Partial-write window — documented in both handlers.

The claim checks out, and there is direct evidence for it in this PR: the writeTx statistic incremented 200 times for a 200-sample request while the handler's outer transaction was open, which only happens if the inner shard commits are independent. Both handlers now carry a note at database.begin() stating that it does not make the request atomic, that TimeSeriesShard.appendSamples commits its own shard writes via getWrappedDatabaseInstance(), and that a later failure cannot roll back batches already written.

Minor (schema lookups before begin()) — intentional and safe. They are metadata reads that need no transaction, and the resolved LocalTimeSeriesType is carried forward in the grouping structure, so the append pass does no second lookup that could observe a different schema.

Verification after these changes: server TS/Prometheus/Grafana/PromQL ITs 58/58; TimeSeriesThreeNodeWalGapReproIT still passes (17.9 s); ha-raft TS ITs 6/6.

Review: batch time-series HTTP ingest

Reviewed the diff against both handlers, TimeSeriesEngine.appendBatch / TimeSeriesShard.appendSamples, and BaseGraphServerTest. This is a clean, well-motivated change - the write-up, inline comments, and test design are all strong. Summary below.

Strengths

• Correct use of the existing primitive. appendBatch already did the round-robin sharding + one-transaction-per-shard grouping and was only reachable from tests; wiring the handlers to it is the right fix rather than inventing new batching. The n==1 fast path keeps single-sample requests cheap.
• Behaviour genuinely preserved. LinkedHashMap/LinkedHashSet keep first-occurrence order, per-sample tag/field resolution is unchanged, and inserted += count keeps the written/dropped accounting consistent with the pre-existing partial-write 400 path. The round-robin shard assignment increments appendCounter once per sample in both the old and new paths, so shard distribution is identical.
• Honest comments. The two NOTE: blocks documenting that the enclosing database.begin() does not make the request atomic (because TimeSeriesShard.appendSamples commits on getWrappedDatabaseInstance()) are exactly what future readers need. This non-atomicity is pre-existing, not introduced here.
• Deterministic test. Using the writeTx stat (commits that produced WAL) as a timing-free proxy for shard append transactions is a nice choice, and reading the shard count back from the auto-created type instead of hardcoding a CPU-derived value makes remoteWriteSeriesIsOneAppendTransactionPerShard portable. The interleaved-measurement and two-series cases cover the grouping logic well.

Prometheus handler micro-optimisation

Resolving each TAG once via findLabelValue + Arrays.fill instead of per-sample is a good call, since findLabelValue re-sanitizes every label name it scans. The column-major fill matches appendBatch's documented [colIdx][sampleIdx] layout. Correct.

Minor / non-blocking notes

1. PR-description/test-name mismatch. The description repeatedly refers to PostTimeSeriesWriteHandlerBatchAppendIT, but the committed file is TimeSeriesIngestBatchAppendIT. Cosmetic, but worth aligning.
2. Duplication between the two handlers. Both now build a long[] timestamps + column-major Object[][] grid and call appendBatch. The sample types differ (LineProtocolParser.Sample vs PrometheusTypes.Sample), so extraction isn't free, but if a third ingest path ever appears this is the spot to factor out. Fine to leave as-is.
3. Failure granularity shifted, as documented. If an appendBatch throws mid-loop, earlier measurements/series are already committed and the caller gets a 500 with a partial write - same shape as before, just at measurement/series rather than sample granularity. The comments call this out; no action needed. The deliberately-flagged RaftHAServer.getTransactionBroker()==null NPE-after-stop() follow-up is a real rough edge worth a tracking issue.
4. Test coverage is standalone (1 node). getServerCount() defaults to 1, so the new IT validates the transaction-count contract but not the HA quorum-round-trip path that motivated the change. TimeSeriesThreeNodeWalGapReproIT covers the HA side, so combined coverage is good - just noting the new IT is a standalone proxy by design.

Nothing blocking from my side. Solid performance fix with a well-targeted regression test.

_{Automated review by Claude Opus 4.8. Reviewed statically; I did not run the build/tests myself.}

Codecov Report

❌ Patch coverage is 92.06349% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 65.28%. Comparing base (c885235) to head (95f212b).
⚠️ Report is 7 commits behind head on main.

@@             Coverage Diff              @@
##               main    #5419      +/-   ##
============================================
+ Coverage     64.82%   65.28%   +0.45%     
- Complexity     1057     1058       +1     
============================================
  Files          1726     1726              
  Lines        143368   143652     +284     
  Branches      30697    30771      +74     
============================================
+ Hits          92934    93777     +843     
+ Misses        37683    37110     -573     
- Partials      12751    12765      +14     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.