You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "nizhikov (via GitHub)" <gi...@apache.org> on 2023/11/29 13:15:43 UTC
[PR] KAFKA-14588 ConfigEntityName move to server-common [kafka]
nizhikov opened a new pull request, #14868:
URL: https://github.com/apache/kafka/pull/14868
This PR is part of "[KAFKA-14588](https://issues.apache.org/jira/browse/KAFKA-14588) Move ConfigCommand to tools".
ConfigCommand depends on ConfigType class.
This PR moves ConfigType 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)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-14588 ConfigEntityName move to server-common [kafka]
Posted by "mimaison (via GitHub)" <gi...@apache.org>.
mimaison commented on PR #14868:
URL: https://github.com/apache/kafka/pull/14868#issuecomment-1876845919
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)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-14588 ConfigEntityName move to server-common [kafka]
Posted by "nizhikov (via GitHub)" <gi...@apache.org>.
nizhikov commented on PR #14868:
URL: https://github.com/apache/kafka/pull/14868#issuecomment-1869386769
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-14588 ConfigEntityName move to server-common [kafka]
Posted by "ijuma (via GitHub)" <gi...@apache.org>.
ijuma commented on PR #14868:
URL: https://github.com/apache/kafka/pull/14868#issuecomment-1877112384
It's a bit weird to move stuff like this to `server-common`. Is this used by anything outside of `tools`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-14588 ConfigEntityName move to server-common [kafka]
Posted by "mimaison (via GitHub)" <gi...@apache.org>.
mimaison merged PR #14868:
URL: https://github.com/apache/kafka/pull/14868
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-14588 ConfigEntityName move to server-common [kafka]
Posted by "ijuma (via GitHub)" <gi...@apache.org>.
ijuma commented on PR #14868:
URL: https://github.com/apache/kafka/pull/14868#issuecomment-1877205588
Sounds good.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-14588 ConfigEntityName move to server-common [kafka]
Posted by "nizhikov (via GitHub)" <gi...@apache.org>.
nizhikov commented on PR #14868:
URL: https://github.com/apache/kafka/pull/14868#issuecomment-1877093884
Test failure was fixed by #417338ad77d15cad2534d7cff436c8776e717255
I merged latest trunk to PR so this must be OK now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-14588 ConfigEntityName move to server-common [kafka]
Posted by "nizhikov (via GitHub)" <gi...@apache.org>.
nizhikov commented on PR #14868:
URL: https://github.com/apache/kafka/pull/14868#issuecomment-1877129273
@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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-14588 ConfigEntityName move to server-common [kafka]
Posted by "mimaison (via GitHub)" <gi...@apache.org>.
mimaison commented on PR #14868:
URL: https://github.com/apache/kafka/pull/14868#issuecomment-1877201295
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-14588 ConfigEntityName move to server-common [kafka]
Posted by "nizhikov (via GitHub)" <gi...@apache.org>.
nizhikov commented on PR #14868:
URL: https://github.com/apache/kafka/pull/14868#issuecomment-1880015968
Double checked test failures.
Looks like not related to the changes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-14588 ConfigEntityName move to server-common [kafka]
Posted by "nizhikov (via GitHub)" <gi...@apache.org>.
nizhikov commented on PR #14868:
URL: https://github.com/apache/kafka/pull/14868#issuecomment-1870012227
@mimaison Can you, please, take a look.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-14588 ConfigEntityName move to server-common [kafka]
Posted by "ijuma (via GitHub)" <gi...@apache.org>.
ijuma commented on PR #14868:
URL: https://github.com/apache/kafka/pull/14868#issuecomment-1877150309
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org