Skip to content

KAFKA-7204: Avoid clearing records for paused partitions on poll of MockConsumer#7505

Merged
guozhangwang merged 1 commit into
apache:trunkfrom
efgpinto:KAFKA-7204
Jan 21, 2020
Merged

KAFKA-7204: Avoid clearing records for paused partitions on poll of MockConsumer#7505
guozhangwang merged 1 commit into
apache:trunkfrom
efgpinto:KAFKA-7204

Conversation

@efgpinto

Copy link
Copy Markdown
Contributor

The previous version of MockConsumer does not allow the clients to test consecutive calls to poll while consuming only from a partial set of partitions due to the fact that it clears all the records after each call. This change makes MockConsumer clearing the records only for the partitions that are not paused (whose records are actually returned by the poll). The remaining paused partitions will retain the records.

Unit test added accordingly.

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

@efgpinto

Copy link
Copy Markdown
Contributor Author

retest this please

@efgpinto

Copy link
Copy Markdown
Contributor Author

@guozhangwang @mjsax @ableegoldman can any of you guys have a look at this MR? I don't think the test failing has anything to do with the changes. Thanks 🙏

@guozhangwang

Copy link
Copy Markdown
Contributor

The proposed fix LGTM, cc @cmccabe @rajinisivaram for another review.

Comment thread clients/src/test/java/org/apache/kafka/clients/consumer/MockConsumerTest.java Outdated
Comment thread clients/src/test/java/org/apache/kafka/clients/consumer/MockConsumerTest.java Outdated
Comment thread clients/src/test/java/org/apache/kafka/clients/consumer/MockConsumerTest.java Outdated

@mjsax mjsax left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM. I would simplify the test a little bit.

@efgpinto

Copy link
Copy Markdown
Contributor Author

Overall LGTM. I would simplify the test a little bit.

Thanks for the review and the suggestions @mjsax . Just pushed a revised version. 👍

@mjsax

mjsax commented Oct 25, 2019

Copy link
Copy Markdown
Member

All three runs failed. Result not available any longer. Could it be a checkstyle issue?

@efgpinto

Copy link
Copy Markdown
Contributor Author

All three runs failed. Result not available any longer. Could it be a checkstyle issue?

Indeed it was. Wildcard import. Solved now. Tks again.

@mjsax

mjsax commented Nov 10, 2019

Copy link
Copy Markdown
Member

Just cycling back to this PR. Seems the Jira ticket was not assigned. I found user epinto (not efgpinto as the GitHub user) -- hope that is correct?

@mjsax

mjsax commented Nov 10, 2019

Copy link
Copy Markdown
Member

Would like to get a review from core team. \cc @cmccabe @rajinisivaram

@efgpinto

Copy link
Copy Markdown
Contributor Author

Just cycling back to this PR. Seems the Jira ticket was not assigned. I found user epinto (not efgpinto as the GitHub user) -- hope that is correct?

It is correct. Thanks.

@mjsax

mjsax commented Dec 1, 2019

Copy link
Copy Markdown
Member

Ping @cmccabe and @rajinisivaram for second review and merging.

@mjsax

mjsax commented Jan 18, 2020

Copy link
Copy Markdown
Member

Ping @hachikuji for second review and merging.

@guozhangwang guozhangwang merged commit c8c0ec6 into apache:trunk Jan 21, 2020
ijuma added a commit to confluentinc/kafka that referenced this pull request Jan 23, 2020
* apache-github/trunk:
  KAFKA-9418; Add new sendOffsetsToTransaction API to KafkaProducer (apache#7952)
  KAFKA-7273 Clarification on mutability of headers passed to Converter#fromConnectData() (apache#7489)
  MINOR: Only update a request's local complete time in API handler if unset (apache#7813)
  KAFKA-9143: Log task reconfiguration error only when it happened (apache#7648)
  MINOR: Change the log level from ERROR to DEBUG when failing to get plugin loader for connector (apache#7964)
  KAFKA-9024: Better error message when field specified does not exist (apache#7819)
  KAFKA-7204: Avoid clearing records for paused partitions on poll of MockConsumer (apache#7505)
  KAFKA-9083: Various fixes/improvements for Connect's Values class (apache#7593)
  MINOR: log error message from Connect sink exception (apache#7555)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants