Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MINOR: Enable some AdminClient integration tests #12110

Merged
merged 3 commits into from May 18, 2022

Conversation

dengziming
Copy link
Member

@dengziming dengziming commented Apr 30, 2022

More detailed description of your change
Enable KRaft in AdminClientWithPoliciesIntegrationTest and PlaintextAdminIntegrationTest
There are some tests not enabled or not as expected yet:

  1. testNullConfigs, see KAFKA-13863
  2. testDescribeCluster and testMetadataRefresh, currently we don't get the real controller in KRaft mode so the test may not run as expected.

Summary of testing strategy (including rationale)
QA

Committer Checklist (excluded from commit message)

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

@dengziming dengziming force-pushed the minor-admin-kraft branch 4 times, most recently from 4e50982 to 6ceb53a Compare May 4, 2022 12:41
@dengziming dengziming force-pushed the minor-admin-kraft branch 3 times, most recently from a9969bd to 1eaeaa1 Compare May 17, 2022 06:53
Copy link
Contributor

@hachikuji hachikuji left a comment

Choose a reason for hiding this comment

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

@dengziming Thanks, just a couple small comments. By the way, let me know if you have any other conversion patches and I can review them.

Copy link
Contributor

@showuon showuon left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. Left some comments.

@showuon
Copy link
Contributor

showuon commented May 18, 2022

Also, the 2 failed tests are introduced by this PR.

Build / JDK 17 and Scala 2.13 / kafka.api.PlaintextAdminIntegrationTest.testDescribeTopicsWithIds(String).quorum=kraft
Build / JDK 17 and Scala 2.13 / kafka.api.PlaintextAdminIntegrationTest.testInvalidIncrementalAlterConfigs(String).quorum=kraft

I checked the topicID issue, it looks like there's a race condition for metadata cache, so that the metadata cache will return ZERO_UUID if empty:
https://github.com/apache/kafka/blob/trunk/core/src/main/scala/kafka/server/metadata/KRaftMetadataCache.scala#L196

Thanks.

@dengziming
Copy link
Member Author

@showuon These 2 tests can be fixed by ensureConsistentKRaftMetadata, I fixed them.

Copy link
Contributor

@hachikuji hachikuji left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM!

@hachikuji hachikuji merged commit 67d00e2 into apache:trunk May 18, 2022
@dengziming dengziming deleted the minor-admin-kraft branch October 8, 2022 12:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants