rajinisivaram commented on a change in pull request #10022:
URL: https://github.com/apache/kafka/pull/10022#discussion_r569292052



##########
File path: 
clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java
##########
@@ -1236,7 +1236,7 @@ public void assign(Collection<TopicPartition> partitions) 
{
                 }
 
                 final FetchedRecords<K, V> records = pollForFetches(timer);
-                if (!records.isEmpty()) {
+                if (!records.records().isEmpty()) {

Review comment:
       Fixing system tests would be straightforward, but as @chia7712 said, 
this could impact existing applications which rely on the current behaviour. 
For example, applications may `poll(longTimeout)` and exit if no records are 
returned, treating it as an error condition, similar to system tests. Adding a 
new API with options sounds like the safe alternative to maintain 
compatibility. But it may be good to follow up on the mailing list, perhaps on 
the discussion thread  of KIP-695 to get wider opinion. Anyway the change 
proposed in this PR seems unsuitable, we can either fix system tests or make 
the new behaviour optional.




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

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


Reply via email to