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

KAFKA-14588 ConfigEntityName move to server-common #14868

Merged
merged 17 commits into from
Jan 8, 2024

Conversation

nizhikov
Copy link
Collaborator

@nizhikov nizhikov commented Nov 29, 2023

This PR is part of "KAFKA-14588 Move ConfigCommand to tools".
ConfigCommand depends on ConfigEntityName class.
This PR moves ConfigEntityName to "server-commons" module.

Changes are relatively huge so it make sense to review then separately from other.

Committer Checklist (excluded from commit message)

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

Copy link
Contributor

@kamalcph kamalcph left a comment

Choose a reason for hiding this comment

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

LGTM!

@nizhikov
Copy link
Collaborator Author

Hello @ijuma @jolshan
Can you, please, take a look?

This PR simply moves ConfigEntityName.
It's required, because ConfigCommand depends on ConfigEntityName and command itself will be moved to tools module.

@nizhikov
Copy link
Collaborator Author

@mimaison Can you, please, take a look.

@mimaison
Copy link
Member

mimaison commented Jan 4, 2024

Running the tests locally I get this failure which seems related:

ConfigCommandTest.shouldNotSupportAlterClientMetricsWithZookeeper()
rg.opentest4j.AssertionFailedError: expected: <client-metrics is not a known entityType. Should be one of List(topics, clients, users, brokers, ips)> but was: <client-metrics is not a known entityType. Should be one of [topics, clients, users, brokers, ips]>
	at app//org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151)
	at app//org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132)
	at app//org.junit.jupiter.api.AssertEquals.failNotEqual(AssertEquals.java:197)
	at app//org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:182)
	at app//org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:177)
	at app//org.junit.jupiter.api.Assertions.assertEquals(Assertions.java:1141)
	at app//kafka.admin.ConfigCommandTest.shouldNotSupportAlterClientMetricsWithZookeeper(ConfigCommandTest.scala:1765)

@nizhikov
Copy link
Collaborator Author

nizhikov commented Jan 4, 2024

Test failure was fixed by 417338a
I merged latest trunk to PR so it must be OK now.

@ijuma
Copy link
Contributor

ijuma commented Jan 4, 2024

It's a bit weird to move stuff like this to server-common. Is this used by anything outside of tools?

@nizhikov
Copy link
Collaborator Author

nizhikov commented Jan 4, 2024

@ijuma

Is this used by anything outside of tools?

AFAICS yes.

DymamicConfig, ClientQuotaMetadataManager, DynamicConfigPublisher to name a few.
These classes works inside the broker isn't it?

@ijuma
Copy link
Contributor

ijuma commented Jan 4, 2024

You're right. The weird part is actually the fact that ConfigCommand relies on this detail instead of using the relevant public apis. I guess it's partly due to the remaining zk code. Seems ok to go with this approach for now and clean it up once we branch for 4.0.

@mimaison
Copy link
Member

mimaison commented Jan 4, 2024

Yes I expect we'll be able to remove that when we remove ZooKeeper. In the meantime, these PRs are helpful to move the commands to the tools module.

@ijuma
Copy link
Contributor

ijuma commented Jan 4, 2024

Sounds good.

@nizhikov
Copy link
Collaborator Author

nizhikov commented Jan 7, 2024

Double checked test failures.
Looks like not related to the changes.

Copy link
Member

@mimaison mimaison left a comment

Choose a reason for hiding this comment

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

LGTM, thanks

@mimaison mimaison merged commit da2aa68 into apache:trunk Jan 8, 2024
1 check failed
yyu1993 pushed a commit to yyu1993/kafka that referenced this pull request Feb 15, 2024
Reviewers: Mickael Maison <mickael.maison@gmail.com>, Kamal Chandraprakash <kamal.chandraprakash@gmail.com>
AnatolyPopov pushed a commit to aiven/kafka that referenced this pull request Feb 16, 2024
Reviewers: Mickael Maison <mickael.maison@gmail.com>, Kamal Chandraprakash <kamal.chandraprakash@gmail.com>
clolov pushed a commit to clolov/kafka that referenced this pull request Apr 5, 2024
Reviewers: Mickael Maison <mickael.maison@gmail.com>, Kamal Chandraprakash <kamal.chandraprakash@gmail.com>
Phuc-Hong-Tran pushed a commit to Phuc-Hong-Tran/kafka that referenced this pull request Jun 6, 2024
Reviewers: Mickael Maison <mickael.maison@gmail.com>, Kamal Chandraprakash <kamal.chandraprakash@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants