Skip to content

MINOR: enable KRaft in ConfigCommandIntegrationTest#11732

Merged
jsancio merged 2 commits into
apache:trunkfrom
ahuang98:kraft-in-config-command-test
Apr 25, 2022
Merged

MINOR: enable KRaft in ConfigCommandIntegrationTest#11732
jsancio merged 2 commits into
apache:trunkfrom
ahuang98:kraft-in-config-command-test

Conversation

@ahuang98

@ahuang98 ahuang98 commented Feb 4, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

Adding KRaft and ZK params to ConfigCommandIntegrationTest wherever appropriate.

Committer Checklist (excluded from commit message)

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

Kvicii
Kvicii approved these changes Feb 8, 2022
View reviewed changes

@Kvicii Kvicii left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

Kvicii
Kvicii reviewed Feb 8, 2022
View reviewed changes

@Kvicii Kvicii left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is there a plan for this PR to be included?

@ahuang98 ahuang98 marked this pull request as ready for review February 8, 2022 19:56
@jsancio jsancio added the kraft label Feb 8, 2022
@ahuang98 ahuang98 force-pushed the kraft-in-config-command-test branch from 6d93c96 to e933b66 Compare February 10, 2022 23:58
dengziming
dengziming approved these changes Feb 12, 2022
View reviewed changes

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

This PR LGTM, however, it seems that we don't support alter user credentials in KRaft mode whereas this test case work fine, maybe we haven't cover that case.

@jsancio

jsancio commented Feb 14, 2022

Copy link
Copy Markdown
Member

@ahuang98 It looks like some of these tests check that the ConfigCommand's command line parser and rejects certain command line arguments. Those tests don't need a ZK or KRaft cluster. We can create a test suite for this command that doesn't create an embedded cluster. This should improve the time for running all of these tests. What do you think?

See core/src/test/scala/unit/kafka/admin/LeaderElectionCommandErrorTest.scala and core/src/test/scala/unit/kafka/admin/LeaderElectionCommandTest.scala for an example of this. Maybe we can apply the same pattern here.

@dengziming

dengziming commented Apr 10, 2022

Copy link
Copy Markdown
Member

We can create a test suite for this command
I did this here #12024 as a convinence.

@ahuang98 ahuang98 force-pushed the kraft-in-config-command-test branch from e933b66 to 003ab44 Compare April 22, 2022 16:29
jsancio
jsancio reviewed Apr 22, 2022
View reviewed changes

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

Thanks for the changes @ahuang98

Comment thread core/src/test/scala/integration/kafka/admin/ConfigCommandIntegrationTest.scala
Comment thread core/src/test/scala/integration/kafka/admin/ConfigCommandIntegrationTest.scala
dengziming
dengziming reviewed Apr 23, 2022
View reviewed changes

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

Thanks for the update, left some comments.

Comment thread core/src/test/scala/unit/kafka/admin/ConfigCommandTest.scala
private val zkConnect = "localhost:2181"
private val dummyAdminZkClient = new DummyAdminZkClient(null)

@Test

@dengziming dengziming Apr 23, 2022

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.

We don't need to add Kraft support here since ConfigCommandTest is just a unit test

Comment thread core/src/test/scala/integration/kafka/admin/ConfigCommandIntegrationTest.scala
Comment thread core/src/test/scala/integration/kafka/admin/ConfigCommandIntegrationTest.scala Outdated

@Test
def shouldExitWithNonZeroStatusOnUpdatingUnallowedConfigViaZk(): Unit = {
@ParameterizedTest

@dengziming dengziming Apr 23, 2022

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.

We can improve display name here, see #11957 for an example

@ahuang98 ahuang98 changed the title MINOR: enable KRaft in ConfigCommandTest MINOR: enable KRaft in ConfigCommandIntegrationTest Apr 25, 2022
@ahuang98 ahuang98 force-pushed the kraft-in-config-command-test branch from ac8f409 to a0b75ac Compare April 25, 2022 17:45
jsancio
jsancio approved these changes Apr 25, 2022
View reviewed changes

@jsancio jsancio 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 the changes @ahuang98. Test failures are unrelated.

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

Reviewers

@dengziming dengziming dengziming approved these changes

@jsancio jsancio jsancio approved these changes

+1 more reviewer

@Kvicii Kvicii Kvicii approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

kraft

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@ahuang98 @jsancio @dengziming @Kvicii