Skip to content

KAFKA-13834: add normal test case#12092

Merged
showuon merged 17 commits into
apache:trunkfrom
ruanliang-hualun:trunk
Apr 24, 2022
Merged

KAFKA-13834: add normal test case#12092
showuon merged 17 commits into
apache:trunkfrom
ruanliang-hualun:trunk

Conversation

@ruanliang-hualun

@ruanliang-hualun ruanliang-hualun commented Apr 24, 2022

Copy link
Copy Markdown
Contributor

@showuon I have add the normal test case, please help to review the pr, thanks

Committer Checklist (excluded from commit message)

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

ruanliang-hualun
ruanliang-hualun commented Apr 24, 2022
View reviewed changes
Comment thread clients/src/test/java/org/apache/kafka/clients/producer/internals/RecordAccumulatorTest.java Outdated

@ruanliang-hualun ruanliang-hualun Apr 24, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

allTpBatchCount represent the count of the producerbatch from the nodeBatches map

ruanliang-hualun
ruanliang-hualun commented Apr 24, 2022
View reviewed changes
Comment thread clients/src/test/java/org/apache/kafka/clients/producer/internals/RecordAccumulatorTest.java Outdated

@ruanliang-hualun ruanliang-hualun Apr 24, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

because the batcheList.size() can be 1 or 2, remove this assert statement

@showuon

showuon commented Apr 24, 2022

Copy link
Copy Markdown
Member

@ruanliang-hualun , thanks for the PR. But as you can see, your changes contain the changes we did last time, and there is merge conflict with trunk branch.
Note: Usually, we'll fork out the branch with the latest trunk code. REF: https://stackoverflow.com/questions/7244321/how-do-i-update-or-sync-a-forked-repository-on-github .

@ruanliang-hualun

ruanliang-hualun commented Apr 24, 2022

Copy link
Copy Markdown
Contributor Author

@showuon Thanks for your patient guidance, I will correct and resubmit it

@ruanliang-hualun

ruanliang-hualun commented Apr 24, 2022

Copy link
Copy Markdown
Contributor Author

@showuon The conflict has been resolved, please help to review the pr, thanks

showuon
showuon approved these changes Apr 24, 2022
View reviewed changes

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

LGTM! Thanks for adding the test!

@showuon

showuon commented Apr 24, 2022

Copy link
Copy Markdown
Member

Failed tests are unrelated.

    Build / JDK 17 and Scala 2.13 / org.apache.kafka.streams.integration.SmokeTestDriverIntegrationTest.shouldWorkWithRebalance
    Build / JDK 8 and Scala 2.12 / kafka.server.DynamicBrokerReconfigurationTest.testAddRemoveSaslListeners()
    Build / JDK 8 and Scala 2.12 / org.apache.kafka.streams.integration.SmokeTestDriverIntegrationTest.shouldWorkWithRebalance

@showuon showuon merged commit e8c675e into apache:trunk Apr 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@showuon showuon showuon approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@ruanliang-hualun @showuon