Skip to content

feat(inkless:consume): coalesce per-batch buffers and isolate corrupt-batch failures [KC-279]#706

Merged
viktorsomogyi merged 4 commits into
mainfrom
jeqo/reader-buffer-coalesce
Jul 17, 2026
Merged

feat(inkless:consume): coalesce per-batch buffers and isolate corrupt-batch failures [KC-279]#706
viktorsomogyi merged 4 commits into
mainfrom
jeqo/reader-buffer-coalesce

Conversation

@jeqo

@jeqo jeqo commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Three focused changes to the diskless consume path in FetchCompleter. Together they cut allocation churn on multi-batch fetches, make the fetch response observable, and stop a single bad batch from taking down an entire multi-partition fetch. No change to the wire protocol or to what a healthy fetch returns.

Motivation

Allocation profiling on the diskless consume path showed FetchCompleter.constructRecordsFromFile as the dominant allocator (~38% on one broker, almost entirely byte[]): it allocated one byte[] per batch and copied each extent intersection into it. Separately, investigating that allocation pressure and a DelayedConsolidationFetch OOM repeatedly required allocation profilers because the fetch path emitted no per-fetch byte-volume or per-partition outcome metrics. Finally, while adding those metrics we found that a batch failing integrity validation threw out of servePartition and aborted every partition in the fetch, unlike classic Kafka which isolates read failures per partition.

What changed

1. Coalesce per-batch buffers (refactor)

extractRecords now allocates one partition-level byte[] sized to the sum of all batch ranges and hands each batch a ByteBuffer.slice() of it, instead of one allocation per batch. Slice capacity is bounded to each batch's byte range, so the in-place header mutations (setLastOffset / setMaxTimestamp) stay confined to that batch's region. The median single-batch fetch is unchanged; the multi-batch tail drops from N allocations to 1. Total bytes allocated are unchanged - this is churn/GC-tail relief, not a bytes reduction. A per-batch fallback covers the (practically unreachable) case where the partition total would overflow an int-sized array.

2. Observability metrics (feat)

FetchCompleter now takes a required (non-null) InklessFetchMetrics and records, under both the consumer (io.aiven.inkless.consume:type=InklessFetchMetrics) and consolidation (io.aiven.inkless.consolidation:type=ConsolidationFetchMetrics) JMX groups:

  • FetchBytesPerFetchSize (Histogram) - total bytes returned per fetch response; combined with the existing partition/batch fan-out counters this gives the full shape of a fetch (partitions x batches x bytes). Empty/error fetches (0 bytes) are skipped so a caught-up long-poll does not flood the histogram low end.
  • PartialPartitionFetchRate (Meter) - partitions whose response was truncated mid-batch-list because a trailing batch was dropped.
  • PartitionStorageErrorRate (Meter) - partitions returned with KAFKA_STORAGE_ERROR (missing metadata or missing/incomplete extents).
  • PartitionCorruptRecordRate (Meter) - partitions returned with CORRUPT_MESSAGE / INVALID_RECORD because a batch failed integrity validation.

docs/inkless/metrics.rst is regenerated via genInklessMetricsDoc.

3. Isolate corrupt-batch failures to the partition (fix)

A batch that failed integrity validation (CRC / declared size, or a coalesced-run offset mismatch) previously propagated out of servePartition, was wrapped in a FetchException, and failed the entire multi-partition fetch - one bad batch in one partition took down every other partition's valid data. This now matches classic Kafka's contract, where ReplicaManager.readFromLocalLog isolates a read failure to its partition (returning the exception's natural code via Errors.forException) and catches unexpected Throwables per partition:

  • constructBatch classifies each batch instead of throwing: missing/non-covering extents -> KAFKA_STORAGE_ERROR; integrity failures -> CorruptRecordException -> CORRUPT_MESSAGE, InvalidRecordException -> INVALID_RECORD. The offset-mismatch / empty-batch guards in createMemoryRecords now raise CorruptRecordException (not IllegalStateException), so a genuine bug is not misclassified as data corruption.
  • servePartition serves the valid prefix as a partial response, or the classified error when nothing was extractable. A per-partition safety net catches any unexpected RuntimeException from extraction (oversized batch, corrupt coordinate, truncated extent) and returns a scoped KAFKA_STORAGE_ERROR, so the "never the whole fetch" contract holds for all failure modes, not just the classified ones.
  • Dropped corrupt batches are logged at WARN with object key and offset range so operators can locate the bad data.
  • Never serves incorrectly-offset data: the corrupt batch is always dropped, never relocated-and-served. On the consumer's next fetch the corrupt batch becomes the first batch and surfaces the classified error directly.

Testing

  • FetchCompleterTest and the full io.aiven.inkless.consume.* suite pass; checkstyle passes.
  • New/updated tests cover: coalesced-buffer slice isolation on setLastOffset; per-fetch byte metric (including the zero-skip guard); partial/storage-error/corrupt-record metric recording; corrupt-batch isolation returning CORRUPT_MESSAGE for the failing partition while a sibling partition is served normally; valid-prefix service on a corrupt trailing batch; and the coalesced offset-mismatch now returning CORRUPT_MESSAGE per partition instead of throwing.
  • Each of the three commits compiles in isolation (the stack is topologically ordered: refactor -> metrics -> isolation).

Notes

  • FetchCompleter no longer has an optional/nullable metrics constructor; the metric collaborator is always present.
  • Total allocated bytes are unchanged by the coalescing; the win is allocation count and GC-tail pressure on multi-batch (lagging / dense-range) fetches. The predicted young-gen pause improvement is not yet measured in production - the new FetchBytesPerFetchSize metric is the tool to validate it.

@jeqo
jeqo force-pushed the jeqo/reader-buffer-coalesce branch from 45ff1a4 to 946fda3 Compare July 17, 2026 10:16
@jeqo
jeqo requested a review from Copilot July 17, 2026 10:18

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR improves the Inkless (diskless) consume/fetch completion path by reducing allocation churn when assembling multi-batch fetches, adding per-fetch/partition observability metrics, and aligning failure isolation with classic Kafka behavior so that a bad batch only fails its own partition response.

Changes:

  • Coalesces per-partition record assembly into a single shared buffer (with per-batch slices) to reduce per-batch allocation churn.
  • Adds new JMX metrics for fetch byte volume and per-partition outcomes (partial fetch, storage error, corrupt record).
  • Changes batch construction/extraction to classify integrity/incompleteness failures per batch and scope failures to the affected partition (serving a valid prefix where possible).

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
storage/inkless/src/main/java/io/aiven/inkless/consume/FetchCompleter.java Implements buffer coalescing, per-fetch byte metric recording, and per-partition error isolation/classification.
storage/inkless/src/main/java/io/aiven/inkless/consume/InklessFetchMetrics.java Introduces new histogram/meter metrics and corresponding recording helpers.
storage/inkless/src/main/java/io/aiven/inkless/consume/Reader.java Wires required InklessFetchMetrics into FetchCompleter.
storage/inkless/src/test/java/io/aiven/inkless/consume/FetchCompleterTest.java Updates constructor usage and adds/updates tests for metrics and isolated corrupt-batch behavior.
docs/inkless/metrics.rst Regenerates metrics documentation to include the new Inkless fetch metrics.

@jeqo
jeqo force-pushed the jeqo/reader-buffer-coalesce branch from 946fda3 to 82e9ea9 Compare July 17, 2026 10:37
@jeqo
jeqo requested a review from Copilot July 17, 2026 10:39

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

@jeqo
jeqo force-pushed the jeqo/reader-buffer-coalesce branch from 82e9ea9 to 79331c1 Compare July 17, 2026 10:50
@jeqo
jeqo requested a review from Copilot July 17, 2026 10:55

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

jeqo and others added 3 commits July 17, 2026 14:35
Allocation profiling on the diskless consume path showed
FetchCompleter.constructRecordsFromFile as the dominant allocator
(~38% on broker 1316, almost entirely byte[]). The hot line allocated
one byte[] per BatchInfo, copied each FileExtent intersection into it,
and wrapped it in MemoryRecords.

Distribution of FetchBatchesPerPartitionBatchCount (2026-06-16):
  p50 < 1, p95 3-4, p99 5-7, p999 12-19.
Median fetches don't drive the cost; multi-batch tail fetches do.

Refactor extractRecords to allocate one partition-level byte[] sized to
the sum of all batch ranges, then hand each batch a slice. Each slice's
capacity is bounded to that batch's byte range, so setLastOffset /
setMaxTimestamp patches stay isolated to that batch's bytes (proven by
the new testCoalescedBufferSliceIsolationOnLastOffsetPatch test). Median
fetch sees no change; p999 drops from ~15 byte[] allocations to 1.

Includes an Integer.MAX_VALUE overflow guard with a per-batch fallback
path; practically unreachable under fetch-byte caps but defensive.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Recent investigations into FetchCompleter allocation pressure and
DelayedConsolidationFetch OOM repeatedly required allocation profilers
because metrics didn't surface (a) per-fetch byte volume distribution
or (b) partition-level completion outcomes.

Add three metrics to InklessFetchMetrics, recorded from FetchCompleter
and emitted under both the consumer JMX group
(io.aiven.inkless.consume:type=InklessFetchMetrics) and the consolidation
JMX group (io.aiven.inkless.consolidation:type=ConsolidationFetchMetrics)
since both paths share the same Reader code:

- FetchResponseSize (Histogram): total bytes returned per fetch response.
  Combined with the existing FetchPartitionsPerFetchCount and
  FetchBatchesPerPartitionCount, gives the full shape of a fetch
  (partitions x batches x bytes). Recorded for every fetch, including
  0-byte responses: the zero mass is itself signal (empty-fetch
  frequency) and keeps it consistent with the other, unguarded
  fetch-shape histograms.
- PartitionPartialFetchRate (Meter): partitions whose response was
  truncated mid-batch-list because a trailing batch lacked an extent or
  failed validation. Today entirely silent; non-zero indicates
  upstream storage-fetch issues delivering partial data.
- PartitionStorageErrorRate (Meter): partitions returned with
  KAFKA_STORAGE_ERROR from FetchCompleter (missing metadata, no
  constructable records). Distinct from per-topic
  failedFetchRequestRate, which conflates local-log and diskless
  completer failures.

FetchCompleter now takes a required (non-null) InklessFetchMetrics; the
metrics are always present rather than optionally wired, removing the
per-call-site null checks. Reader passes fetchMetrics; tests pass a mock.

Regenerates docs/inkless/metrics.rst via genInklessMetricsDoc.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A batch that fails integrity validation previously propagated out of
servePartition through serveFetch, was wrapped in a FetchException, and
failed the entire multi-partition fetch. One bad batch in one partition
took down every other partition's valid data in the same response.

This matches classic Kafka's contract: ReplicaManager.readFromLocalLog
isolates a read failure to its partition (returning the exception's
natural code via Errors.forException) and catches unexpected Throwables
per-partition, never aborting the whole fetch. Bring diskless in line:

- constructBatch classifies each batch instead of throwing. Missing or
  non-covering extents -> KAFKA_STORAGE_ERROR; integrity failures -> the
  classic per-record code (CorruptRecordException -> CORRUPT_MESSAGE,
  InvalidRecordException -> INVALID_RECORD). The coalesced-run offset
  mismatch and empty-physical-batch guards in createMemoryRecords now
  raise CorruptRecordException (not IllegalStateException), so a genuine
  bug is no longer misclassified as data corruption.
- An invalid batch size from a corrupt coordinate (negative, or exceeding
  Integer.MAX_VALUE) is classified in the extraction loop as
  CORRUPT_MESSAGE and keeps the valid prefix, rather than throwing into
  the safety net and discarding it.
- servePartition serves the valid prefix as a partial response, or the
  classified error when nothing was extractable. A per-partition safety
  net still catches any other unexpected RuntimeException from extraction
  (e.g. a truncated extent) and returns a scoped KAFKA_STORAGE_ERROR, so
  the "never the whole fetch" contract holds for all failure modes, not
  just the classified ones.
- Dropped corrupt batches are logged at WARN with object key and offset
  range so operators can locate the bad data (previously only an
  aggregate rate meter fired).
- Add PartitionCorruptRecordRate meter for terminal CORRUPT_MESSAGE /
  INVALID_RECORD outcomes, symmetric with PartitionStorageErrorRate
  (both terminal-only; truncation is covered by PartialPartitionFetchRate).

Never serves incorrectly-offset data: the corrupt batch is always
dropped, never relocated-and-served. The per-batch coalesce/per-batch
buffer paths are unified into a single loop.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@jeqo
jeqo force-pushed the jeqo/reader-buffer-coalesce branch from 79331c1 to 06812b5 Compare July 17, 2026 11:42
@jeqo
jeqo marked this pull request as ready for review July 17, 2026 11:43
@jeqo
jeqo requested a review from viktorsomogyi July 17, 2026 11:43

@viktorsomogyi viktorsomogyi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the commits are sound, just had one question regarding logging.

if (rawSize < 0 || rawSize > Integer.MAX_VALUE) {
// Corrupt coordinate (nonsensical size). Classify as CORRUPT_MESSAGE ("exceeds the
// valid size") and keep the valid prefix, rather than throwing into the safety net.
LOGGER.warn("Dropping diskless batch with invalid size {} for {} in object {}",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How would a consumer react to a CORRUPT_MESSAGE? Is it retriable by the consumer? Would it flood the logs?

} catch (CorruptRecordException | InvalidRecordException e) {
// Data corruption (bad CRC / size, or a coalesced-run offset mismatch): log object +
// offsets so operators can locate it, drop the batch, serve the prefix or an error.
LOGGER.warn("Dropping corrupt diskless batch for {} in object {} offsets [{}, {}]: {}",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question as above, can it flood the log?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch, I have fallback to DEBUG and keep the metric as the main signal. PTAL

@jeqo
jeqo requested a review from viktorsomogyi July 17, 2026 14:22
@viktorsomogyi
viktorsomogyi merged commit 1156792 into main Jul 17, 2026
5 checks passed
@viktorsomogyi
viktorsomogyi deleted the jeqo/reader-buffer-coalesce branch July 17, 2026 15:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants