rajinisivaram commented on a change in pull request #11448:
URL: https://github.com/apache/kafka/pull/11448#discussion_r740091138
##########
File path: core/src/main/scala/kafka/server/DynamicBrokerConfig.scala
##########
@@ -516,7 +516,15 @@ class DynamicBrokerConfig(private val kafkaConfig:
KafkaConfig) extends Logging
newProps ++= staticBrokerConfigs
overrideProps(newProps, dynamicDefaultConfigs)
overrideProps(newProps, dynamicBrokerConfigs)
- val oldConfig = currentConfig
+
+ // We need a copy of the current config since `currentConfig` is
initialized with `kafkaConfig`
+ // which means the call to `updateCurrentConfig` would end up mutating
`oldConfig`.
+ val oldConfig = if (kafkaConfig eq currentConfig) {
Review comment:
@hachikuji Thanks for the PR. Do you know if there is an issue only with
KRraft or is there an issue with ZK as well?
With ZK, the sequence is:
1. Create KafkaConfig with static configs from server.properties
2. Initialize KafkaConfig with initial configs from ZooKeeper using
`DynamicBrokerConfig.initialize(zkClient)`. This always creates a new
KafkaConfig, so we don't need this check?
3. Start DynamicConfigManager. All ZK updates after 2) are handled through
change notifications.
With KRaft, there doesn't seem to be an initialize() for initializing the
state from existing dynamic configs, so are all configs handled similar to 3)?
In which case, we should perhaps move the initialization of `currentConfig`
from `DynamicBrokerConfig.initialize(zkClient)` to somewhere common for KRaft
and avoid this check here?
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
