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/08/11 20:34:39 UTC
[GitHub] [kafka] rondagostino opened a new pull request #11200: KAFKA-13192: Prevent inconsistent broker.id/node.id values
rondagostino opened a new pull request #11200:
URL: https://github.com/apache/kafka/pull/11200
If both `broker.id` and `node.id` are set, and they are set inconsistently (e.g.`broker.id=0`, `node.id=1`) then currently the value of `node.id` is used and the `broker.id` value is left at the original value. The server should detect this inconsistency, throw a ConfigException, and fail to start. This patch adds the check and a test for it.
### 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
[GitHub] [kafka] jsancio commented on a change in pull request #11200: KAFKA-13192: Prevent inconsistent broker.id/node.id values
Posted by GitBox <gi...@apache.org>.
jsancio commented on a change in pull request #11200:
URL: https://github.com/apache/kafka/pull/11200#discussion_r687192965
##########
File path: core/src/main/scala/kafka/server/KafkaConfig.scala
##########
@@ -1420,6 +1420,17 @@ class KafkaConfig(val props: java.util.Map[_, _], doLog: Boolean, dynamicConfigO
override def get(key: String): AnyRef =
if (this eq currentConfig) super.get(key) else currentConfig.get(key)
+ override def postProcessParsedConfig(props: java.util.Map[String,Object]): java.util.Map[String,Object] = {
Review comment:
Why are we doing the validation in this method since it always returns an empty map? Can we do the same validation in `validateValues`? `KafkaConfig` performs the more complicated validation in that 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] rondagostino commented on a change in pull request #11200: KAFKA-13192: Prevent inconsistent broker.id/node.id values
Posted by GitBox <gi...@apache.org>.
rondagostino commented on a change in pull request #11200:
URL: https://github.com/apache/kafka/pull/11200#discussion_r688016827
##########
File path: core/src/main/scala/kafka/server/KafkaConfig.scala
##########
@@ -1905,6 +1905,13 @@ class KafkaConfig(val props: java.util.Map[_, _], doLog: Boolean, dynamicConfigO
@nowarn("cat=deprecation")
private def validateValues(): Unit = {
+ val nodeIdValue = values.get(KafkaConfig.NodeIdProp).asInstanceOf[Int]
+ if (nodeIdValue >= 0) {
+ val brokerIdValue = values.get(KafkaConfig.BrokerIdProp).asInstanceOf[Int]
Review comment:
> prefers to use getInt
God catch -- fixed.
--
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] jsancio commented on a change in pull request #11200: KAFKA-13192: Prevent inconsistent broker.id/node.id values
Posted by GitBox <gi...@apache.org>.
jsancio commented on a change in pull request #11200:
URL: https://github.com/apache/kafka/pull/11200#discussion_r687854292
##########
File path: core/src/main/scala/kafka/server/KafkaConfig.scala
##########
@@ -1905,6 +1905,13 @@ class KafkaConfig(val props: java.util.Map[_, _], doLog: Boolean, dynamicConfigO
@nowarn("cat=deprecation")
private def validateValues(): Unit = {
+ val nodeIdValue = values.get(KafkaConfig.NodeIdProp).asInstanceOf[Int]
+ if (nodeIdValue >= 0) {
+ val brokerIdValue = values.get(KafkaConfig.BrokerIdProp).asInstanceOf[Int]
Review comment:
```suggestion
val nodeIdValue = getInt(KafkaConfig.NodeIdProp)
if (nodeIdValue >= 0) {
val brokerIdValue = getInt(KafkaConfig.BrokerIdProp)
```
For consistency, this type prefers to use `getInt` which does the casting.
--
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] rondagostino commented on pull request #11200: KAFKA-13192: Prevent inconsistent broker.id/node.id values
Posted by GitBox <gi...@apache.org>.
rondagostino commented on pull request #11200:
URL: https://github.com/apache/kafka/pull/11200#issuecomment-905514929
This PR is likely going to be replaced by https://github.com/apache/kafka/pull/11256. Will probably eventually close this assuming that other PR gets merged.
--
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] rondagostino commented on a change in pull request #11200: KAFKA-13192: Prevent inconsistent broker.id/node.id values
Posted by GitBox <gi...@apache.org>.
rondagostino commented on a change in pull request #11200:
URL: https://github.com/apache/kafka/pull/11200#discussion_r688016827
##########
File path: core/src/main/scala/kafka/server/KafkaConfig.scala
##########
@@ -1905,6 +1905,13 @@ class KafkaConfig(val props: java.util.Map[_, _], doLog: Boolean, dynamicConfigO
@nowarn("cat=deprecation")
private def validateValues(): Unit = {
+ val nodeIdValue = values.get(KafkaConfig.NodeIdProp).asInstanceOf[Int]
+ if (nodeIdValue >= 0) {
+ val brokerIdValue = values.get(KafkaConfig.BrokerIdProp).asInstanceOf[Int]
Review comment:
> prefers to use getInt
Good catch -- fixed.
--
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] rondagostino commented on a change in pull request #11200: KAFKA-13192: Prevent inconsistent broker.id/node.id values
Posted by GitBox <gi...@apache.org>.
rondagostino commented on a change in pull request #11200:
URL: https://github.com/apache/kafka/pull/11200#discussion_r688014091
##########
File path: core/src/main/scala/kafka/server/KafkaConfig.scala
##########
@@ -1905,6 +1905,13 @@ class KafkaConfig(val props: java.util.Map[_, _], doLog: Boolean, dynamicConfigO
@nowarn("cat=deprecation")
private def validateValues(): Unit = {
+ val nodeIdValue = values.get(KafkaConfig.NodeIdProp).asInstanceOf[Int]
+ if (nodeIdValue >= 0) {
+ val brokerIdValue = values.get(KafkaConfig.BrokerIdProp).asInstanceOf[Int]
+ if (brokerIdValue != Defaults.BrokerId && brokerIdValue != nodeIdValue) {
+ throw new ConfigException(s"The values for broker.id ($brokerIdValue) and node.id ($nodeIdValue) must be the same if both are specified")
+ }
+ }
Review comment:
Good question. It's legal to set node.id for the ZooKeeper case, so I think this inconsistency check is applicable in all cases and should be done here, at the top.
--
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] rondagostino commented on a change in pull request #11200: KAFKA-13192: Prevent inconsistent broker.id/node.id values
Posted by GitBox <gi...@apache.org>.
rondagostino commented on a change in pull request #11200:
URL: https://github.com/apache/kafka/pull/11200#discussion_r687199159
##########
File path: core/src/main/scala/kafka/server/KafkaConfig.scala
##########
@@ -1420,6 +1420,17 @@ class KafkaConfig(val props: java.util.Map[_, _], doLog: Boolean, dynamicConfigO
override def get(key: String): AnyRef =
if (this eq currentConfig) super.get(key) else currentConfig.get(key)
+ override def postProcessParsedConfig(props: java.util.Map[String,Object]): java.util.Map[String,Object] = {
Review comment:
Good point! I'll move 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] showuon commented on a change in pull request #11200: KAFKA-13192: Prevent inconsistent broker.id/node.id values
Posted by GitBox <gi...@apache.org>.
showuon commented on a change in pull request #11200:
URL: https://github.com/apache/kafka/pull/11200#discussion_r687624316
##########
File path: core/src/main/scala/kafka/server/KafkaConfig.scala
##########
@@ -1905,6 +1905,13 @@ class KafkaConfig(val props: java.util.Map[_, _], doLog: Boolean, dynamicConfigO
@nowarn("cat=deprecation")
private def validateValues(): Unit = {
+ val nodeIdValue = values.get(KafkaConfig.NodeIdProp).asInstanceOf[Int]
+ if (nodeIdValue >= 0) {
+ val brokerIdValue = values.get(KafkaConfig.BrokerIdProp).asInstanceOf[Int]
+ if (brokerIdValue != Defaults.BrokerId && brokerIdValue != nodeIdValue) {
+ throw new ConfigException(s"The values for broker.id ($brokerIdValue) and node.id ($nodeIdValue) must be the same if both are specified")
+ }
+ }
Review comment:
Do you think we can put this check into below else block for KRaft-based metadata quorum case only?
--
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