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/07/31 19:31:27 UTC
[GitHub] [kafka] dielhennr opened a new pull request #11159: MINOR: Change default node id in kraft broker properties
dielhennr opened a new pull request #11159:
URL: https://github.com/apache/kafka/pull/11159
Previously, the default `node.id` specified in `config/kraft/controller.properties` was the same as the `node.id` specified in `config/kraft/broker.properties`. If the user didn't change one of the `node.id`s by themselves then the broker would never catch up to the metadata log after registering with the controller. Hence, never unfenced itself. There is a [debug statement](https://github.com/apache/kafka/blob/8a1fcee86e42c8bd1f26309dde8748927109056e/core/src/main/scala/kafka/server/BrokerLifecycleManager.scala#L375) in `BrokerLifecycleManager` that tells the user that the broker has not caught up with the metadata log, but it does not explicitly say that the broker attempting to register is using the `node.id` of a controller.
This PR changes the default `node.id` in `config/kraft/broker.properties` from 1 to 2 so that it can be used with `config/kraft/controller.properties` by default.
--
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 pull request #11159: MINOR: Fix logging in ClusterControlManager
Posted by GitBox <gi...@apache.org>.
jsancio commented on pull request #11159:
URL: https://github.com/apache/kafka/pull/11159#issuecomment-1030979984
> @jsancio are you going to merge this?
Yes. I can merge it. @dielhennr can you resolve the conflicts?
--
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 closed pull request #11159: MINOR: Fix logging in ClusterControlManager
Posted by GitBox <gi...@apache.org>.
jsancio closed pull request #11159:
URL: https://github.com/apache/kafka/pull/11159
--
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 #11159: MINOR: Change default node id in kraft broker properties
Posted by GitBox <gi...@apache.org>.
jsancio commented on a change in pull request #11159:
URL: https://github.com/apache/kafka/pull/11159#discussion_r682775395
##########
File path: core/src/main/scala/kafka/server/ControllerApis.scala
##########
@@ -552,7 +552,10 @@ class ControllerApis(val requestChannel: RequestChannel,
def handleBrokerRegistration(request: RequestChannel.Request): Unit = {
val registrationRequest = request.body[BrokerRegistrationRequest]
authHelper.authorizeClusterOperation(request, CLUSTER_ACTION)
-
+ val controllerNode = controllerNodes.find(node => node.id == registrationRequest.brokerId)
+ if (controllerNode.isDefined) {
+ info(s"A broker is registering with ${KafkaConfig.NodeIdProp}=${controllerNode.get.id} which is also in use by a controller.")
+ }
Review comment:
I would remove this log message. This is a valid configuration and I don't think it is providing any additional information.
##########
File path: clients/src/main/java/org/apache/kafka/common/requests/BrokerRegistrationRequest.java
##########
@@ -57,6 +57,10 @@ public BrokerRegistrationRequestData data() {
return data;
}
+ public int brokerId() {
+ return data.brokerId();
+ }
+
Review comment:
I think this pattern undermines the advantages of the message JSON schema and the Java code generated from 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] jsancio commented on a change in pull request #11159: MINOR: Change default node id in kraft broker properties
Posted by GitBox <gi...@apache.org>.
jsancio commented on a change in pull request #11159:
URL: https://github.com/apache/kafka/pull/11159#discussion_r682775395
##########
File path: core/src/main/scala/kafka/server/ControllerApis.scala
##########
@@ -552,7 +552,10 @@ class ControllerApis(val requestChannel: RequestChannel,
def handleBrokerRegistration(request: RequestChannel.Request): Unit = {
val registrationRequest = request.body[BrokerRegistrationRequest]
authHelper.authorizeClusterOperation(request, CLUSTER_ACTION)
-
+ val controllerNode = controllerNodes.find(node => node.id == registrationRequest.brokerId)
+ if (controllerNode.isDefined) {
+ info(s"A broker is registering with ${KafkaConfig.NodeIdProp}=${controllerNode.get.id} which is also in use by a controller.")
+ }
Review comment:
I would remove this log message. This is a valid configuration and I don't think it is providing any additional information.
##########
File path: clients/src/main/java/org/apache/kafka/common/requests/BrokerRegistrationRequest.java
##########
@@ -57,6 +57,10 @@ public BrokerRegistrationRequestData data() {
return data;
}
+ public int brokerId() {
+ return data.brokerId();
+ }
+
Review comment:
I think this pattern undermines the advantages of the message JSON schema and the Java code generated from 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] dielhennr edited a comment on pull request #11159: MINOR: Change default node id in kraft broker properties
Posted by GitBox <gi...@apache.org>.
dielhennr edited a comment on pull request #11159:
URL: https://github.com/apache/kafka/pull/11159#issuecomment-890412999
If all quorum voters wrote a `RegisterVoterRecord` or similar to the metadata quorum with the `node.id` and `process.roles` of that node, then controllers would be able to triage invalid brokers registrations that use the same `node.id` as a controller. This is because if they are using the same `node.id` but we also know that `process.roles=broker,controller` then it would be valid configuration.
--
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 #11159: MINOR: Change default node id in kraft broker properties
Posted by GitBox <gi...@apache.org>.
jsancio commented on a change in pull request #11159:
URL: https://github.com/apache/kafka/pull/11159#discussion_r681283772
##########
File path: metadata/src/main/java/org/apache/kafka/controller/ClusterControlManager.java
##########
@@ -239,11 +239,11 @@ public void replay(RegisterBrokerRecord record) {
features.put(feature.name(), new VersionRange(
feature.minSupportedVersion(), feature.maxSupportedVersion()));
}
+ BrokerRegistration prevRegistration = brokerRegistrations.getOrDefault(brokerId, null);
Review comment:
Yes, it looks like it affects the log message printed, right? It doesn't explain the behavior you are seeing.
--
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] dielhennr removed a comment on pull request #11159: MINOR: Change default node id in kraft broker properties
Posted by GitBox <gi...@apache.org>.
dielhennr removed a comment on pull request #11159:
URL: https://github.com/apache/kafka/pull/11159#issuecomment-890412999
If all quorum voters wrote a `RegisterVoterRecord` or similar to the metadata quorum with the `node.id` and `process.roles` of that node, then controllers would be able to triage invalid brokers registrations that use the same `node.id` as a controller. This is because if they are using the same `node.id` but we also know that `process.roles=broker,controller` then it would be valid configuration.
--
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 #11159: MINOR: Change default node id in kraft broker properties
Posted by GitBox <gi...@apache.org>.
jsancio commented on a change in pull request #11159:
URL: https://github.com/apache/kafka/pull/11159#discussion_r681159222
##########
File path: metadata/src/main/java/org/apache/kafka/controller/ClusterControlManager.java
##########
@@ -239,11 +239,11 @@ public void replay(RegisterBrokerRecord record) {
features.put(feature.name(), new VersionRange(
feature.minSupportedVersion(), feature.maxSupportedVersion()));
}
+ BrokerRegistration prevRegistration = brokerRegistrations.getOrDefault(brokerId, null);
Review comment:
`Map::put` returns the previous value.
--
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] dielhennr commented on a change in pull request #11159: MINOR: Change default node id in kraft broker properties
Posted by GitBox <gi...@apache.org>.
dielhennr commented on a change in pull request #11159:
URL: https://github.com/apache/kafka/pull/11159#discussion_r681972655
##########
File path: metadata/src/main/java/org/apache/kafka/controller/ClusterControlManager.java
##########
@@ -239,11 +239,11 @@ public void replay(RegisterBrokerRecord record) {
features.put(feature.name(), new VersionRange(
feature.minSupportedVersion(), feature.maxSupportedVersion()));
}
+ BrokerRegistration prevRegistration = brokerRegistrations.getOrDefault(brokerId, null);
Review comment:
Yes it affects the logging.
--
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] dielhennr closed pull request #11159: MINOR: Change default node id in kraft broker properties
Posted by GitBox <gi...@apache.org>.
dielhennr closed pull request #11159:
URL: https://github.com/apache/kafka/pull/11159
--
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] dielhennr commented on pull request #11159: MINOR: Change default node id in kraft broker properties
Posted by GitBox <gi...@apache.org>.
dielhennr commented on pull request #11159:
URL: https://github.com/apache/kafka/pull/11159#issuecomment-890412999
If all quorum voters wrote a `RegisterVoterRecord` or similar to the metadata quorum with the `node.id` and `process.roles` of that node, then controllers would be able to triage invalid clusters where brokers register with the same `node.id` as a controller.
--
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] ijuma commented on pull request #11159: MINOR: Fix logging in ClusterControlManager
Posted by GitBox <gi...@apache.org>.
ijuma commented on pull request #11159:
URL: https://github.com/apache/kafka/pull/11159#issuecomment-1030533071
@jsancio are you going to merge this?
--
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] dielhennr commented on a change in pull request #11159: MINOR: Change default node id in kraft broker properties
Posted by GitBox <gi...@apache.org>.
dielhennr commented on a change in pull request #11159:
URL: https://github.com/apache/kafka/pull/11159#discussion_r681202052
##########
File path: metadata/src/main/java/org/apache/kafka/controller/ClusterControlManager.java
##########
@@ -239,11 +239,11 @@ public void replay(RegisterBrokerRecord record) {
features.put(feature.name(), new VersionRange(
feature.minSupportedVersion(), feature.maxSupportedVersion()));
}
+ BrokerRegistration prevRegistration = brokerRegistrations.getOrDefault(brokerId, null);
Review comment:
This was still a bug then correct? `prevRegistration` was being assigned to `Map::get` not `Map::put`.
--
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] dielhennr closed pull request #11159: MINOR: Change default node id in kraft broker properties
Posted by GitBox <gi...@apache.org>.
dielhennr closed pull request #11159:
URL: https://github.com/apache/kafka/pull/11159
--
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 pull request #11159: MINOR: Fix logging in ClusterControlManager
Posted by GitBox <gi...@apache.org>.
jsancio commented on pull request #11159:
URL: https://github.com/apache/kafka/pull/11159#issuecomment-1030981141
This was already fixed in trunk. Closing the PR.
--
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