[GitHub] [kafka] sudeshwasnik commented on a diff in pull request #12462: [KAFKA-9965] Fix accumulator tryAppend, so that fresh new producerBatch is created

[GitBox <gi...@apache.org>]

sudeshwasnik commented on code in PR #12462:
URL: https://github.com/apache/kafka/pull/12462#discussion_r962663696


##########
clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordAccumulator.java:
##########
@@ -273,26 +273,29 @@ public RecordAppendResult append(String topic,
 
                 // check if we have an in-progress batch
                 Deque<ProducerBatch> dq = topicInfo.batches.computeIfAbsent(effectivePartition, k -> new ArrayDeque<>());
+                RecordAppendResult appendResult;
                 synchronized (dq) {
                     // After taking the lock, validate that the partition hasn't changed and retry.
                     if (topicInfo.builtInPartitioner.isPartitionChanged(partitionInfo)) {
                         log.trace("Partition {} for topic {} switched by a concurrent append, retrying",
                                 partitionInfo.partition(), topic);
                         continue;
                     }
-                    RecordAppendResult appendResult = tryAppend(timestamp, key, value, headers, callbacks, dq, nowMs);
-                    if (appendResult != null) {
+                    appendResult = tryAppend(timestamp, key, value, headers, callbacks, dq, nowMs);
+                    if (appendResult != null && !appendResult.newBatchCreated) {
                         topicInfo.builtInPartitioner.updatePartitionInfo(partitionInfo, appendResult.appendedBytes, cluster);
                         return appendResult;
                     }
                 }
 
-                // we don't have an in-progress record batch try to allocate a new batch
-                if (abortOnNewBatch) {
-                    // Return a result that will cause another call to append.
+                // either 1. current topicPartition producerBatch is full - return and prepare for another batch/partition.
+                // 2. no producerBatch existed for this topicPartition, create a new producerBatch.
+                if (appendResult == null && abortOnNewBatch) {

Review Comment:
   Makes sense! with current approach we could get stuck to same partition, leading to starvation. 
   There are now two constraints - 1. Prevent even partitions getting skipped consistently when there are no producerBatches corresponding to this partition. 2. Do not always select the same partition when producerBatch is empty. ie, detect if recent drain was done for this partition and skip it in that case. 



-- 
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: jira-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org