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 16:59:39 UTC
[PR] KAFKA-14588 Move DynamicConfig to server-commons [kafka]
nizhikov opened a new pull request, #14871:
URL: https://github.com/apache/kafka/pull/14871
This PR move `DynamicConfig` class to "server-commons" module.
### 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 Move DynamicConfig to server-commons [kafka]
Posted by "nizhikov (via GitHub)" <gi...@apache.org>.
nizhikov commented on PR #14871:
URL: https://github.com/apache/kafka/pull/14871#issuecomment-1881376286
> To get the correct data, the tool has to ask the server via RPC. Am I missing something?
Different version of Kafka supports different dynamic properties.
So, to create correct list of supported properties we must get it from server.
If yes, I will create separate Jira for this.
> Even DynamicConfig seems odd to be in server-common instead of server.
Got it. Thanks.
--
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 Move DynamicConfig to server-commons [kafka]
Posted by "ijuma (via GitHub)" <gi...@apache.org>.
ijuma commented on PR #14871:
URL: https://github.com/apache/kafka/pull/14871#issuecomment-1881237874
Even `DynamicConfig` seems odd to be in `server-common` instead of `server`. Things that are specific to the broker should not be in `server-common`.
--
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 Move DynamicConfig to server-commons [kafka]
Posted by "nizhikov (via GitHub)" <gi...@apache.org>.
nizhikov commented on PR #14871:
URL: https://github.com/apache/kafka/pull/14871#issuecomment-1880989880
Hello. @mmaison
Please, take a look here before reviewing #15075
--
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 Move DynamicConfig to server-commons [kafka]
Posted by "nizhikov (via GitHub)" <gi...@apache.org>.
nizhikov commented on PR #14871:
URL: https://github.com/apache/kafka/pull/14871#issuecomment-1833482403
Hello @ijuma
Can you, please, take a look?
This PR is first in the series of changes to move `ConfigCommand` to tools module.
It moves `ConfigType`, `ConfigEntityName`, `DynamicConfig` classes to `server-common` 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 Move DynamicConfig to server-commons [kafka]
Posted by "mimaison (via GitHub)" <gi...@apache.org>.
mimaison commented on PR #14871:
URL: https://github.com/apache/kafka/pull/14871#issuecomment-1881434444
Note there's also https://github.com/apache/kafka/pull/15103 which is attempting to move KafkaConfig to the server module. You may want to sync with Omnia to avoid duplicating work.
--
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 Move DynamicConfig to server-commons [kafka]
Posted by "nizhikov (via GitHub)" <gi...@apache.org>.
nizhikov closed pull request #14871: KAFKA-14588 Move DynamicConfig to server-commons
URL: https://github.com/apache/kafka/pull/14871
--
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 Move DynamicConfig to server-commons [kafka]
Posted by "nizhikov (via GitHub)" <gi...@apache.org>.
nizhikov commented on PR #14871:
URL: https://github.com/apache/kafka/pull/14871#issuecomment-1881211127
@mimaison @ijuma Guys, I need your advice here.
`ConfigCommand` code are tightly coupled with other part of `core` module.
In this PR I tried to move one of dependency - `DynamicConfig` to `server-commons`.
There still one method that not moved:
```
object Broker {
DynamicBrokerConfig.addDynamicConfigs(BROKER_CONFIG_DEF)
val nonDynamicProps = KafkaConfig.configNames.toSet -- BROKER_CONFIG_DEF.names.asScala
}
```
dependencies are `DynamicConfig` -> `DynamicBrokerConfig` -> `KafkaConfig`, etc.
Can you, please, advice me - should I continue and try move all chain to java?
I can try to make separate PR with moving `KafkaConfig` and other classes.
Or is it too much for now and I must go other way for these 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 Move DynamicConfig to server-commons [kafka]
Posted by "nizhikov (via GitHub)" <gi...@apache.org>.
nizhikov commented on PR #14871:
URL: https://github.com/apache/kafka/pull/14871#issuecomment-1881277689
@ijuma We can use direct calls like `QuotaConfigs.userAndClientQuotaConfigs().names()`, `QuotaConfigs.scramMechanismsPlusUserAndClientQuotaConfigs().names()` to obtain config names for help message in `ConfigCommand`.
But I can't see how `DynamicConfig.Broker.names` can be created without accessing to `KafkaConfig.configKeys`
```
object DynamicConfig {
object Broker {
...
DynamicBrokerConfig.addDynamicConfigs(brokerConfigDef)
}
}
object DynamicBrokerConfig {
private[server] def addDynamicConfigs(configDef: ConfigDef): Unit = {
KafkaConfig.configKeys.forKeyValue { (configName, config) =>
if (AllDynamicConfigs.contains(configName)) {
configDef.define(config.name, config.`type`, config.defaultValue, config.validator,
config.importance, config.documentation, config.group, config.orderInGroup, config.width,
config.displayName, config.dependents, config.recommender)
}
}
}
}
``
--
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 Move DynamicConfig to server-commons [kafka]
Posted by "ijuma (via GitHub)" <gi...@apache.org>.
ijuma commented on PR #14871:
URL: https://github.com/apache/kafka/pull/14871#issuecomment-1881328863
It seems like the help message is wrong though, right? It will print the values based on the tools version versus the server version. Properly separating this code will surface bugs like this. To get the correct data, the tool has to ask the server via RPC.
--
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 Move DynamicConfig to server-commons [kafka]
Posted by "ijuma (via GitHub)" <gi...@apache.org>.
ijuma commented on PR #14871:
URL: https://github.com/apache/kafka/pull/14871#issuecomment-1881235062
KafkaConfig should move to `server`, not `server-common`. We can introduce new interfaces if needed to disentangle the dependencies. Moving a large part of `core` to `server-common` defeats the modularization effort.
--
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 Move DynamicConfig to server-commons [kafka]
Posted by "ijuma (via GitHub)" <gi...@apache.org>.
ijuma commented on PR #14871:
URL: https://github.com/apache/kafka/pull/14871#issuecomment-1881344819
By the way, if some of this disentangling is too hard to do all at once - it may actually be easier to rewrite in Java and move it to the `server` module. Then we can take another step where we move the pieces from `server` to `server-common`. If we bloat `server-common`, I think it will be harder to make it slim again later.
--
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