[GitHub] [kafka] vvcephei commented on pull request #9836: KAFKA-10866: Add metadata to ConsumerRecords

2021-02-02 Thread GitBox 


vvcephei commented on pull request #9836:
URL: https://github.com/apache/kafka/pull/9836#issuecomment-772140902


   Thanks @ijuma ; We did run the Streams system tests and benchmarks and 
verified there was no perf regression. I should have run the Client system 
tests too; sorry about that. I'll follow up on 
https://github.com/apache/kafka/pull/10022



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

[GitHub] [kafka] vvcephei commented on pull request #9836: KAFKA-10866: Add metadata to ConsumerRecords

2021-01-27 Thread GitBox 


vvcephei commented on pull request #9836:
URL: https://github.com/apache/kafka/pull/9836#issuecomment-768664736


   Flaky test failures:
   
   ```
   Build / JDK 11 / 
org.apache.kafka.clients.consumer.internals.FetcherTest.testEarlierOffsetResetArrivesLate()
   Build / JDK 11 / 
org.apache.kafka.clients.producer.KafkaProducerTest.testHeadersWithExtendedClasses()
   Build / JDK 15 / 
kafka.integration.MetricsDuringTopicCreationDeletionTest.testMetricsDuringTopicCreateDelete()
   ```
   
   The most concerning one is the FetcherTest, but it's also failing on trunk.



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

[GitHub] [kafka] vvcephei commented on pull request #9836: KAFKA-10866: Add metadata to ConsumerRecords

2021-01-27 Thread GitBox 


vvcephei commented on pull request #9836:
URL: https://github.com/apache/kafka/pull/9836#issuecomment-768546193


   Most of those failures were known flaky tests, but one was an EasyMock 
error. I'm not able to repro it locally after a rebase, though. Rebased, 
pushed, and trying one more time to get a clean build.



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

[GitHub] [kafka] vvcephei commented on pull request #9836: KAFKA-10866: Add metadata to ConsumerRecords

2021-01-26 Thread GitBox 


vvcephei commented on pull request #9836:
URL: https://github.com/apache/kafka/pull/9836#issuecomment-767869047


   That last build was green, but it was a few days ago. Just rebased and 
pushed again to make sure I don't break the build when I merge this.



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

[GitHub] [kafka] vvcephei commented on pull request #9836: KAFKA-10866: Add metadata to ConsumerRecords

2021-01-22 Thread GitBox 


vvcephei commented on pull request #9836:
URL: https://github.com/apache/kafka/pull/9836#issuecomment-765602186


   Thanks @guozhangwang . I share that concern.
   
   It's a little different than what you pointed out, but to call attention to 
it: I actually didn't change the definition of `ConsumerRecords#isEmpty`. It is 
still just `return records.isEmpty()`, so it is only true when there are 
records. The specific rationale is not to break logic similar to what you 
quoted.
   
   To your point, it's true that `M > N`, but I doubt that `M >> N`. The rate 
at which we return is still bounded by the `latency * concurrency` of fetch 
request-response lifecycles _that return some records or metadata_. As long as 
there's no new information to return, the Consumer's long-poll will still be 
effective.
   
   And to the final point, I really doubt that any real code could have an 
expectation that they can just call `poll` and _always_ get nonzero records 
back. It just doesn't seem like a reasonable expectation from a user's 
perspective.



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

[GitHub] [kafka] vvcephei commented on pull request #9836: KAFKA-10866: Add metadata to ConsumerRecords

2021-01-22 Thread GitBox 


vvcephei commented on pull request #9836:
URL: https://github.com/apache/kafka/pull/9836#issuecomment-765574234


   Thanks @guozhangwang ,
   
   I considered that, but I think for this work, we actually do want to return 
the metadata if we have fetched some. Since the semantics of `poll(timeout)` 
are that it will wait _up to_ the timeout to return and that it may or may not 
return any records, it seems this PR's behavior is just fine.
   
   All that is wrong is that this particular test expects to get records back 
after a single poll. Among our integration tests, this is a pretty unique 
expectation, so I felt good about relaxing it. Does that seem ok to you?



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

[GitHub] [kafka] vvcephei commented on pull request #9836: KAFKA-10866: Add metadata to ConsumerRecords

2021-01-22 Thread GitBox 


vvcephei commented on pull request #9836:
URL: https://github.com/apache/kafka/pull/9836#issuecomment-765507001


   Thanks, @guozhangwang ! 
   
   I ran the PlaintextConsumerTest a bunch more times, and also searched 
through the trunk build logs. I think this PR did make the test more flaky, and 
I suspect the reason is that the consumer long-poll will now return "early" if 
we get a metadata-only fetch response. I've adjusted the test to account for 
this, and we'll see how the build does now.



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

[GitHub] [kafka] vvcephei commented on pull request #9836: KAFKA-10866: Add metadata to ConsumerRecords

2021-01-21 Thread GitBox 


vvcephei commented on pull request #9836:
URL: https://github.com/apache/kafka/pull/9836#issuecomment-764973767


   Huh, I can't get the PlaintextConsumerTest to fail locally...



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

[GitHub] [kafka] vvcephei commented on pull request #9836: KAFKA-10866: Add metadata to ConsumerRecords

2021-01-21 Thread GitBox 


vvcephei commented on pull request #9836:
URL: https://github.com/apache/kafka/pull/9836#issuecomment-764973767


   Huh, I can't get the PlaintextConsumerTest to fail locally...



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