[GitHub] [kafka] artemlivshits commented on a diff in pull request #12570: KAFKA-14156: Built-in partitioner may create suboptimal batches

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

artemlivshits commented on code in PR #12570:
URL: https://github.com/apache/kafka/pull/12570#discussion_r964385141


##########
clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordAccumulator.java:
##########
@@ -378,6 +415,16 @@ private MemoryRecordsBuilder recordsBuilder(ByteBuffer buffer, byte maxUsableMag
     }
 
     /**
+     * Check if there are ready batches in the queue, or we sent all batches.
+     */
+    private boolean queueHasReadyBatches(Deque<ProducerBatch> deque, long nowMs) {
+        // Note that we also check if the queue is empty, because that may mean that batches became
+        // ready and we sent them.
+        ProducerBatch last = deque.peekLast();
+        return deque.size() > 1 || last == null || last.isFull() || last.waitedTimeMs(nowMs) >= lingerMs;

Review Comment:
   "Fractional" batches in built-in partitioner could happen with lingerMs=0 as well, and it was probably this consideration that led to the DefaultPartitioner implementation, which switches partitions on batch boundary.  The problem with letting the batch fill based on back pressure (which is pretty much the only driver for batching when linger.ms=0) is that we're likely to form larger batches to a slower broker and exhibit the original problem described in KAFKA-10888.  This change is carefully avoiding any signals that could be based on backpressure and lets the batch fill only if it's not ready because it doesn't meet readiness criteria itself (full or waited for linger.ms), e.g. we don't let batch fill if there are other ready batches in the queue -- that means the broker wasn't ready to take a ready batch, i.e. backpressure signal.
   
   That said, we could introduce some heuristics, e.g. switch partition if the batch becomes ready and we are within 10% (configurable) of sticky limit, so that instead of starting a new batch in the partition that we switch away from soon, we would start a new batch in the new partition where it may have better opportunity to fill.  This would reintroduce some of the KAFKA-10888 pattern, but hopefully the 10% would put a bound on that.  The downside of this approach is that it would make the logic even more complex to model and configure, and it's a little research project in itself.  I was hoping to get some data from real-life usage of built-in partitioner and see if we need to build the heuristics (as a new project) or the current logic provides good default behavior.



-- 
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