feat(inkless:consume): coalesce per-batch buffers and isolate corrupt-batch failures [KC-279]#706
Conversation
45ff1a4 to
946fda3
Compare
There was a problem hiding this comment.
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. |
946fda3 to
82e9ea9
Compare
82e9ea9 to
79331c1
Compare
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>
79331c1 to
06812b5
Compare
| 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 {}", |
There was a problem hiding this comment.
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 [{}, {}]: {}", |
There was a problem hiding this comment.
Same question as above, can it flood the log?
There was a problem hiding this comment.
good catch, I have fallback to DEBUG and keep the metric as the main signal. PTAL
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.constructRecordsFromFileas the dominant allocator (~38% on one broker, almost entirelybyte[]): it allocated onebyte[]per batch and copied each extent intersection into it. Separately, investigating that allocation pressure and aDelayedConsolidationFetchOOM 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 ofservePartitionand aborted every partition in the fetch, unlike classic Kafka which isolates read failures per partition.What changed
1. Coalesce per-batch buffers (
refactor)extractRecordsnow allocates one partition-levelbyte[]sized to the sum of all batch ranges and hands each batch aByteBuffer.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)FetchCompleternow takes a required (non-null)InklessFetchMetricsand 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 withKAFKA_STORAGE_ERROR(missing metadata or missing/incomplete extents).PartitionCorruptRecordRate(Meter) - partitions returned withCORRUPT_MESSAGE/INVALID_RECORDbecause a batch failed integrity validation.docs/inkless/metrics.rstis regenerated viagenInklessMetricsDoc.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 aFetchException, 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, whereReplicaManager.readFromLocalLogisolates a read failure to its partition (returning the exception's natural code viaErrors.forException) and catches unexpectedThrowables per partition:constructBatchclassifies 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 increateMemoryRecordsnow raiseCorruptRecordException(notIllegalStateException), so a genuine bug is not misclassified as data corruption.servePartitionserves the valid prefix as a partial response, or the classified error when nothing was extractable. A per-partition safety net catches any unexpectedRuntimeExceptionfrom extraction (oversized batch, corrupt coordinate, truncated extent) and returns a scopedKAFKA_STORAGE_ERROR, so the "never the whole fetch" contract holds for all failure modes, not just the classified ones.Testing
FetchCompleterTestand the fullio.aiven.inkless.consume.*suite pass; checkstyle passes.setLastOffset; per-fetch byte metric (including the zero-skip guard); partial/storage-error/corrupt-record metric recording; corrupt-batch isolation returningCORRUPT_MESSAGEfor the failing partition while a sibling partition is served normally; valid-prefix service on a corrupt trailing batch; and the coalesced offset-mismatch now returningCORRUPT_MESSAGEper partition instead of throwing.Notes
FetchCompleterno longer has an optional/nullable metrics constructor; the metric collaborator is always present.FetchBytesPerFetchSizemetric is the tool to validate it.