junrao commented on code in PR #20285:
URL: https://github.com/apache/kafka/pull/20285#discussion_r2411324898
##########
clients/src/main/java/org/apache/kafka/clients/producer/internals/ProducerBatch.java:
##########
@@ -237,6 +238,20 @@ public boolean completeExceptionally(
return done(ProduceResponse.INVALID_OFFSET, RecordBatch.NO_TIMESTAMP,
topLevelException, recordExceptions);
}
+ /**
+ * Get all record futures for this batch.
+ * This is used by flush() to wait on individual records rather than the
batch-level future.
+ * When batches are split, individual futures are chained to the new
batches,
+ * ensuring flush() waits for all split batches to complete.
+ *
+ * @return List of FutureRecordMetadata for all records in this batch
+ */
+ public List<FutureRecordMetadata> recordFutures() {
+ return thunks.stream()
+ .map(thunk -> thunk.future)
Review Comment:
Ok, then it's probably useful to do the optimized approach. We will need to
add a list of depending ProduceRequestResult in ProduceRequestResult.
ProduceRequestResult.await() will further wait for the depending
ProduceRequestResult after local latch is unblocked. In ProducerBatch.split(),
we collect all ProduceRequestResult in batches and chain them to produceFuture.
##########
clients/src/test/java/org/apache/kafka/clients/producer/internals/RecordAccumulatorTest.java:
##########
@@ -1790,4 +1825,56 @@ public void
testSplitAndReenqueuePreventInfiniteRecursion() throws InterruptedEx
// Verify all original records are accounted for (no data loss)
assertEquals(100, keyFoundMap.size(), "All original 100 records should
be present after splitting");
}
+
+ @Test
+ public void testFlushPerformanceWithManyRecords() throws Exception {
Review Comment:
Could we turn this into a jmh test and add it to the jmh-benchmarks
directory?
##########
clients/src/test/java/org/apache/kafka/clients/producer/internals/RecordAccumulatorTest.java:
##########
@@ -1066,6 +1066,41 @@ public void testSplitAndReenqueue() throws
ExecutionException, InterruptedExcept
assertEquals(1, future2.get().offset());
}
+ // here I am testing the hasRoomFor() behaviour
Review Comment:
Could we add a unit test that fails without the fix?
--
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]
