Skip to content

KAFKA-16938: non-dynamic props gets corrupted due to circular reference between DynamicBrokerConfig and DynamicConfig. #16302

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.

Sign up for GitHub

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

Merged
chia7712 merged 5 commits into from
Jun 13, 2024
Merged

KAFKA-16938: non-dynamic props gets corrupted due to circular reference between DynamicBrokerConfig and DynamicConfig. #16302

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0658846
KAFKA-16938: Break circular reference between DynamicBrokerConfig and…
frankvicky Jun 12, 2024
88ac773
KAFKA-16938: Add comment and correct indentation
frankvicky Jun 12, 2024
a59fa3f
KAFKA-16938: Remove unused method addDynamicConfigs
frankvicky Jun 12, 2024
e27c225
KAFKA-16938: Rewrite comment to make it more precise
frankvicky Jun 12, 2024
40e992c
KAFKA-16938: Format comment
frankvicky Jun 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 2 additions & 14 deletions core/src/main/scala/kafka/server/DynamicBrokerConfig.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,8 +117,6 @@ object DynamicBrokerConfig {
AllDynamicConfigs.intersect(passwordConfigs)
}

private val nonDynamicProps: Set[String] = KafkaConfig.configNames.toSet -- DynamicConfig.Broker.names.asScala

def isPasswordConfig(name: String): Boolean = DynamicBrokerConfig.DynamicPasswordConfigs.exists(name.endsWith)

def brokerConfigSynonyms(name: String, matchListenerOverride: Boolean): List[String] = {
Expand Down Expand Up @@ -168,7 +166,7 @@ object DynamicBrokerConfig {
}

private def nonDynamicConfigs(props: Properties): Set[String] = {
props.asScala.keySet.intersect(nonDynamicProps)
props.asScala.keySet.intersect(DynamicConfig.Broker.nonDynamicProps)
}

private def securityConfigsWithoutListenerPrefix(props: Properties): Set[String] = {
Expand All @@ -184,16 +182,6 @@ object DynamicBrokerConfig {
DynamicConfig.Broker.validate(baseProps)
}

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)
}
}
}

private[server] def dynamicConfigUpdateModes: util.Map[String, String] = {
AllDynamicConfigs.map { name =>
val mode = if (PerBrokerConfigs.contains(name)) "per-broker" else "cluster-wide"
Expand Down Expand Up @@ -321,7 +309,7 @@ class DynamicBrokerConfig(private val kafkaConfig: KafkaConfig) extends Logging
}

private def verifyReconfigurableConfigs(configNames: Set[String]): Unit = CoreUtils.inWriteLock(lock) {
val nonDynamic = configNames.intersect(nonDynamicProps)
val nonDynamic = configNames.intersect(DynamicConfig.Broker.nonDynamicProps)
require(nonDynamic.isEmpty, s"Reconfigurable contains non-dynamic configs $nonDynamic")
}

Expand Down
18 changes: 15 additions & 3 deletions core/src/main/scala/kafka/server/DynamicConfig.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@

package kafka.server

import kafka.server.DynamicBrokerConfig.AllDynamicConfigs

import java.net.{InetAddress, UnknownHostException}
import java.util.Properties
import org.apache.kafka.common.config.ConfigDef
Expand All @@ -30,10 +32,20 @@ import scala.jdk.CollectionConverters._
* and can only be set dynamically.
*/
object DynamicConfig {
object Broker {
private val brokerConfigs = {
val configs = QuotaConfigs.brokerQuotaConfigs()

// Filter and define all dynamic configurations
KafkaConfig.configKeys
.filter { case (configName, _) => AllDynamicConfigs.contains(configName) }

@chia7712 chia7712 Jun 12, 2024

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.

Could you please add comments to remind readers about the circular reference?

.foreach { case (_, config) => configs.define(config) }
configs
}

object Broker {
private val brokerConfigs = QuotaConfigs.brokerQuotaConfigs()
DynamicBrokerConfig.addDynamicConfigs(brokerConfigs)

@mimaison mimaison Jun 12, 2024

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.

So DynamicBrokerConfig.addDynamicConfigs() is not used anymore, we can delete it.

@frankvicky frankvicky Jun 12, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thank you, I will remove it 😄

// Non-dynamic properties are determined by subtracting dynamic broker config names from all config names.

@chia7712 chia7712 Jun 12, 2024

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.

The key point is DynamicBrokerConfig should NOT have any val variables which are initialized by DynamicConfig.Broker. Hence, the comments can be tweaked to

In order to avoid circular reference, all DynamicBrokerConfig's variables which are initialized by `DynamicConfig.Broker` should be moved to `DynamicConfig.Broker`. Otherwise, those variables of DynamicBrokerConfig will see intermediate state of `DynamicConfig.Broker`, because `brokerConfigs` is created by `DynamicBrokerConfig.AllDynamicConfigs`

@frankvicky frankvicky Jun 12, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I see, I will modify it soon.

// This is to avoid circular reference issues during initialization.
val nonDynamicProps: Set[String] = KafkaConfig.configNames.toSet -- brokerConfigs.names.asScala

def configKeys: util.Map[String, ConfigDef.ConfigKey] = brokerConfigs.configKeys

Expand Down