KAFKA-20913: Snapshot CompletedBatch appendTimestamp before buffer reuse - #23311
Open
shantanushukla wants to merge 1 commit into
Open
KAFKA-20913: Snapshot CompletedBatch appendTimestamp before buffer reuse#23311shantanushukla wants to merge 1 commit into
shantanushukla wants to merge 1 commit into
Conversation
CompletedBatch.appendTimestamp() read maxTimestamp from MemoryRecords that may share a pooled buffer. After release(), a later batch can overwrite that header, so the leader commit callback delivered the wrong append time. Store the timestamp when the completed batch is constructed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
JIRA: https://issues.apache.org/jira/browse/KAFKA-20913
On the Raft leader,
KafkaRaftClient.appendBatchwrites the batch to the log, registers an async commit callback, thenrelease()s the pooled buffer.CompletedBatch.appendTimestamp()was readingMemoryRecords.firstBatch().maxTimestamp()from a slice of that buffer. Production usesBatchMemoryPool, which requeues the buffer; a later batch overwrites the v2 header. When HWM later completes the callback,handleCommitcan deliver the newer batch's append time.Fix
Snapshot
maxTimestampwhenCompletedBatchis constructed and return that field fromappendTimestamp(). The timestamp is fixed at build time (BatchBuilderwritestime.milliseconds()into the header). Record payloads andsizeInBytes()are not affected.Why existing tests missed this
RaftClientTestContextdefaults toMemoryPool.NONE, which allocates a new buffer every time and never reuses.BatchAccumulatorTestcalledappendTimestamp()beforerelease(), or verifiedrelease()without reading the timestamp afterward.Tests
These failed on trunk (first batch reported T2) and pass after the snapshot:
BatchAccumulatorTest.testAppendTimestampUnchangedAfterBufferReleasedAndReused— one-bufferBatchMemoryPool; drain/release at T1, append at T2; first batch must still report T1.KafkaRaftClientTest.testLeaderHandleCommitAppendTimestampStableWhenPoolReusesBuffer— two voters so HWM lags; catch the listener up past the leader-change batch; append at T1 then T2 with a single reused buffer; after commit the listener's first data batch must still be T1.Related existing tests still pass: full
BatchAccumulatorTest,testListenerCommitCallbackAfterLeaderWrite,testAccumulatorClearedAfterBecomingFollower.BUILD SUCCESSFUL.