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/05/24 21:12:27 UTC
[GitHub] [kafka] jolshan opened a new pull request #10754: KAFKA-12835: Topic IDs can mismatch on brokers (after interbroker protocol version update)
jolshan opened a new pull request #10754:
URL: https://github.com/apache/kafka/pull/10754
Upon upgrading to IBP 2.8, topic ID can end up getting reassigned which can cause errors in LeaderAndIsr handling when the partition metadata files from the previous ID are still on the broker.
Topic IDs are stored in the TopicZNode. The behavior of the code before this fix is as follows:
When we have a controller with an older IBP version and we reassign partitions, the TopicZNode is overwritten and we lose the topic ID. Upon electing a 2.8 IBP controller, we will see the TopicZNode is missing a topic ID and will generate a new one. If the broker still has the old partition metadata file, we will see an ID mismatch that causes the error.
This PR changes controller logic so that we maintain the topic ID in the controller and the ZNode even when IBP < 2.8. This means that in the scenario above, reassigning partitions will not result in losing the topic ID and reassignment.
Topic IDs may be lost when downgrading the code below version 2.8, but upon re-upgrading to code version 2.8, before bumping the IBP, all partition metadata files will be deleted to prevent any errors.
### 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dajac merged pull request #10754: KAFKA-12835: Topic IDs can mismatch on brokers (after interbroker protocol version update)
Posted by GitBox <gi...@apache.org>.
dajac merged pull request #10754:
URL: https://github.com/apache/kafka/pull/10754
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] lbradstreet commented on a change in pull request #10754: KAFKA-12835: Topic IDs can mismatch on brokers (after interbroker protocol version update)
Posted by GitBox <gi...@apache.org>.
lbradstreet commented on a change in pull request #10754:
URL: https://github.com/apache/kafka/pull/10754#discussion_r651822091
##########
File path: core/src/main/scala/kafka/log/Log.scala
##########
@@ -337,6 +337,9 @@ class Log(@volatile private var _dir: File,
}
} else if (keepPartitionMetadataFile) {
_topicId.foreach(partitionMetadataFile.write)
+ } else {
Review comment:
This was a pre-existing issue but should we extract this whole if else block to a method? It's also a little bit easy to miss that it's setting the `_topicid` in https://github.com/apache/kafka/pull/10754/files#diff-eeafed82ed6a8600c397b108787fdf31e03191b0a192774a65c127d0d26edc44L330. Maybe that could benefit from an empty line prior 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jolshan commented on a change in pull request #10754: KAFKA-12835: Topic IDs can mismatch on brokers (after interbroker protocol version update)
Posted by GitBox <gi...@apache.org>.
jolshan commented on a change in pull request #10754:
URL: https://github.com/apache/kafka/pull/10754#discussion_r652022729
##########
File path: core/src/main/scala/kafka/log/Log.scala
##########
@@ -337,6 +337,9 @@ class Log(@volatile private var _dir: File,
}
} else if (keepPartitionMetadataFile) {
_topicId.foreach(partitionMetadataFile.write)
+ } else {
Review comment:
Yeah. Unfortunately there is a lot going on here. I've edited the comment and spacing to make it a little clearer. If we still think it needs adjustment, I can try making a new 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] lbradstreet commented on a change in pull request #10754: KAFKA-12835: Topic IDs can mismatch on brokers (after interbroker protocol version update)
Posted by GitBox <gi...@apache.org>.
lbradstreet commented on a change in pull request #10754:
URL: https://github.com/apache/kafka/pull/10754#discussion_r651818541
##########
File path: core/src/main/scala/kafka/controller/KafkaController.scala
##########
@@ -1658,10 +1658,21 @@ class KafkaController(val config: KafkaConfig,
}
private def processTopicIds(topicIdAssignments: Set[TopicIdReplicaAssignment]): Unit = {
- if (config.usesTopicId) {
- val updated = zkClient.setTopicIds(topicIdAssignments.filter(_.topicId.isEmpty), controllerContext.epochZkVersion)
- val allTopicIdAssignments = updated ++ topicIdAssignments.filter(_.topicId.isDefined)
- allTopicIdAssignments.foreach(topicIdAssignment => controllerContext.addTopicId(topicIdAssignment.topic, topicIdAssignment.topicId.get))
+ // Create topic IDs for topics missing them if we are using topic IDs
+ // Otherwise, maintain what we have in the topicZNode
+ val updated = if (config.usesTopicId) {
+ zkClient.setTopicIds(topicIdAssignments.filter(_.topicId.isEmpty), controllerContext.epochZkVersion)
+ } else {
+ Set[TopicIdReplicaAssignment]()
+ }
+
+ // Add topic IDs to controller context
+ // If we don't have IBP 2.8, but are running 2.8 code, put any topic IDs from the ZNode in controller context
+ // This is to avoid losing topic IDs during operations like partition reassignments while the cluster is in a mixed state
+ (updated ++ topicIdAssignments).foreach{ topicIdAssignment =>
Review comment:
nit: `foreach{` to `foreach {`
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jolshan commented on pull request #10754: KAFKA-12835: Topic IDs can mismatch on brokers (after interbroker protocol version update)
Posted by GitBox <gi...@apache.org>.
jolshan commented on pull request #10754:
URL: https://github.com/apache/kafka/pull/10754#issuecomment-847389546
Hmm looks like `KafkaMetadataLogTest.testTopicId` is failing because we set `keepPartitionMetadataFile` to be false. When I ensure that we only assign topicId when `keepPartitionMetadataFile` we do not assign topic ID to the log. Since we rely on assignment in memory + in the file to be consistent, one option is to write a partition.metadata file for the metadata topic. This won't be used like the other partition.metadata files, but it might be easier to keep all logs consistent.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jolshan commented on pull request #10754: KAFKA-12835: Topic IDs can mismatch on brokers (after interbroker protocol version update)
Posted by GitBox <gi...@apache.org>.
jolshan commented on pull request #10754:
URL: https://github.com/apache/kafka/pull/10754#issuecomment-861710587
I thought I cherry-picked this to 2.8, but I guess I didn't. I will need to do that as well.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dajac commented on pull request #10754: KAFKA-12835: Topic IDs can mismatch on brokers (after interbroker protocol version update)
Posted by GitBox <gi...@apache.org>.
dajac commented on pull request #10754:
URL: https://github.com/apache/kafka/pull/10754#issuecomment-863820766
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] lbradstreet commented on a change in pull request #10754: KAFKA-12835: Topic IDs can mismatch on brokers (after interbroker protocol version update)
Posted by GitBox <gi...@apache.org>.
lbradstreet commented on a change in pull request #10754:
URL: https://github.com/apache/kafka/pull/10754#discussion_r652042564
##########
File path: core/src/main/scala/kafka/log/Log.scala
##########
@@ -337,6 +337,9 @@ class Log(@volatile private var _dir: File,
}
} else if (keepPartitionMetadataFile) {
_topicId.foreach(partitionMetadataFile.write)
+ } else {
Review comment:
I think extracting it into a method named initializeTopicId or something like that would be good.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org