You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2021/09/08 22:34:51 UTC
[GitHub] [kafka] cmccabe opened a new pull request #11312: KAFKA-13224: Ensure that broker.id is set in KafkaConfig#originals
cmccabe opened a new pull request #11312:
URL: https://github.com/apache/kafka/pull/11312
Some plugins make use of KafkaConfig#originals rather than the
KafkaConfig object. We should ensure that these plugins see the
correct value for broker.id if the broker is running in KRaft mode and
node.id has been configured, but not broker.id.
This PR does this by ensuring that both node.id and broker.id are set in
the orignals map if either one is set. We also check that they are set
to the same value in KafkaConfig#validateValues.
Co-author: Ron Dagostino <rd...@confluent.io>
--
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
[GitHub] [kafka] cmccabe commented on a change in pull request #11312: KAFKA-13224: Ensure that broker.id is set in KafkaConfig#originals
Posted by GitBox <gi...@apache.org>.
cmccabe commented on a change in pull request #11312:
URL: https://github.com/apache/kafka/pull/11312#discussion_r706506964
##########
File path: core/src/main/scala/kafka/server/KafkaConfig.scala
##########
@@ -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)
Review comment:
AbstractConfig already copies the map which is passed to 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
[GitHub] [kafka] mumrah commented on a change in pull request #11312: KAFKA-13224: Ensure that broker.id is set in KafkaConfig#originals
Posted by GitBox <gi...@apache.org>.
mumrah commented on a change in pull request #11312:
URL: https://github.com/apache/kafka/pull/11312#discussion_r706419226
##########
File path: core/src/main/scala/kafka/server/KafkaConfig.scala
##########
@@ -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)
Review comment:
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?
##########
File path: core/src/main/scala/kafka/server/KafkaConfig.scala
##########
@@ -1385,10 +1385,22 @@ object KafkaConfig {
}
if (maybeSensitive) Password.HIDDEN else value
}
+
+ def populateSynonyms(input: util.Map[_, _]): util.Map[Any, Any] = {
Review comment:
A comment or docstring would be useful here explaining the motivation for this method
--
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
[GitHub] [kafka] hachikuji commented on a change in pull request #11312: KAFKA-13224: Ensure that broker.id is set in KafkaConfig#originals
Posted by GitBox <gi...@apache.org>.
hachikuji commented on a change in pull request #11312:
URL: https://github.com/apache/kafka/pull/11312#discussion_r706502119
##########
File path: core/src/main/scala/kafka/server/KafkaConfig.scala
##########
@@ -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)
+ 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 {
Review comment:
It seems a little bit odd that we populate synonyms only in the reference that we're passing to `AbstractConfig`. The field `KafkaConfig.props` could still be accessed directly (maybe it should be private?). Would it make sense to move this to a factory method? For example:
```scala
object KafkaConfig {
def apply(props: java.util.Map[_, _], doLog: Boolean, dynamicConfigOverride: Option[DynamicBrokerConfig]): KafkaConfig = {
new KafkaConfig(populateSynonyms(props), doLog, dynamicConfigOverride)
}
}
```
--
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
[GitHub] [kafka] cmccabe commented on a change in pull request #11312: KAFKA-13224: Ensure that broker.id is set in KafkaConfig#originals
Posted by GitBox <gi...@apache.org>.
cmccabe commented on a change in pull request #11312:
URL: https://github.com/apache/kafka/pull/11312#discussion_r706507377
##########
File path: core/src/main/scala/kafka/server/KafkaConfig.scala
##########
@@ -1385,10 +1385,22 @@ object KafkaConfig {
}
if (maybeSensitive) Password.HIDDEN else value
}
+
+ def populateSynonyms(input: util.Map[_, _]): util.Map[Any, Any] = {
Review comment:
Added.
--
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
[GitHub] [kafka] cmccabe merged pull request #11312: KAFKA-13224: Ensure that broker.id is set in KafkaConfig#originals
Posted by GitBox <gi...@apache.org>.
cmccabe merged pull request #11312:
URL: https://github.com/apache/kafka/pull/11312
--
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
[GitHub] [kafka] cmccabe commented on a change in pull request #11312: KAFKA-13224: Ensure that broker.id is set in KafkaConfig#originals
Posted by GitBox <gi...@apache.org>.
cmccabe commented on a change in pull request #11312:
URL: https://github.com/apache/kafka/pull/11312#discussion_r706509007
##########
File path: core/src/main/scala/kafka/server/KafkaConfig.scala
##########
@@ -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)
+ 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 {
Review comment:
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 `KafkaConfig#populateSynonyms` in all the (publicly visible) secondary constructors. This should fix 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