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-13224: Ensure that broker.id is set in KafkaConfig#originals #11312
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1385,10 +1385,22 @@ object KafkaConfig { | |
} | ||
if (maybeSensitive) Password.HIDDEN else value | ||
} | ||
|
||
def populateSynonyms(input: util.Map[_, _]): util.Map[Any, Any] = { | ||
val output = new util.HashMap[Any, Any](input) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it safe to make a new map here? Are there any callers of KafkaConfig() which might expect to modify the props map after the object has been constructed? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. AbstractConfig already copies the map which is passed to it. |
||
val brokerId = output.get(KafkaConfig.BrokerIdProp) | ||
val nodeId = output.get(KafkaConfig.NodeIdProp) | ||
if (brokerId == null && nodeId != null) { | ||
output.put(KafkaConfig.BrokerIdProp, nodeId) | ||
} else if (brokerId != null && nodeId == null) { | ||
output.put(KafkaConfig.NodeIdProp, brokerId) | ||
} | ||
output | ||
} | ||
} | ||
|
||
class KafkaConfig(val props: java.util.Map[_, _], doLog: Boolean, dynamicConfigOverride: Option[DynamicBrokerConfig]) | ||
extends AbstractConfig(KafkaConfig.configDef, props, doLog) with Logging { | ||
extends AbstractConfig(KafkaConfig.configDef, KafkaConfig.populateSynonyms(props), doLog) with Logging { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems a little bit odd that we populate synonyms only in the reference that we're passing to object KafkaConfig {
def apply(props: java.util.Map[_, _], doLog: Boolean, dynamicConfigOverride: Option[DynamicBrokerConfig]): KafkaConfig = {
new KafkaConfig(populateSynonyms(props), doLog, dynamicConfigOverride)
}
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, good find. This is a case where Scala's behavior is kind of annoying. I wish there was a way to opt-out of the auto-initialization. Anyway, I made the primary constructor private, and put a call to |
||
|
||
def this(props: java.util.Map[_, _]) = this(props, true, None) | ||
def this(props: java.util.Map[_, _], doLog: Boolean) = this(props, doLog, None) | ||
|
@@ -1516,15 +1528,8 @@ class KafkaConfig(val props: java.util.Map[_, _], doLog: Boolean, dynamicConfigO | |
/** ********* General Configuration ***********/ | ||
val brokerIdGenerationEnable: Boolean = getBoolean(KafkaConfig.BrokerIdGenerationEnableProp) | ||
val maxReservedBrokerId: Int = getInt(KafkaConfig.MaxReservedBrokerIdProp) | ||
var brokerId: Int = { | ||
val nodeId = getInt(KafkaConfig.NodeIdProp) | ||
if (nodeId < 0) { | ||
getInt(KafkaConfig.BrokerIdProp) | ||
} else { | ||
nodeId | ||
} | ||
} | ||
val nodeId: Int = brokerId | ||
var brokerId: Int = getInt(KafkaConfig.BrokerIdProp) | ||
val nodeId: Int = getInt(KafkaConfig.NodeIdProp) | ||
val processRoles: Set[ProcessRole] = parseProcessRoles() | ||
val initialRegistrationTimeoutMs: Int = getInt(KafkaConfig.InitialBrokerRegistrationTimeoutMsProp) | ||
val brokerHeartbeatIntervalMs: Int = getInt(KafkaConfig.BrokerHeartbeatIntervalMsProp) | ||
|
@@ -1905,6 +1910,9 @@ class KafkaConfig(val props: java.util.Map[_, _], doLog: Boolean, dynamicConfigO | |
|
||
@nowarn("cat=deprecation") | ||
private def validateValues(): Unit = { | ||
if (nodeId != brokerId) { | ||
throw new ConfigException(s"You must set `${KafkaConfig.NodeIdProp}` to the same value as `${KafkaConfig.BrokerIdProp}`.") | ||
} | ||
if (requiresZookeeper) { | ||
if (zkConnect == null) { | ||
throw new ConfigException(s"Missing required configuration `${KafkaConfig.ZkConnectProp}` which has no default value.") | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A comment or docstring would be useful here explaining the motivation for this method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added.