KAFKA-12403; Ensure local state deleted on RemoveTopicRecord received#10252
Conversation
chia7712
left a comment
chia7712
left a comment
There was a problem hiding this comment.
@hachikuji Thanks for this fix. a couple of comments. Please take a look.
| record.asInstanceOf[RemoveTopicRecord]) | ||
| case QUOTA_RECORD => handleQuotaRecord(imageBuilder, | ||
| record.asInstanceOf[QuotaRecord]) | ||
| // TODO: handle FEATURE_LEVEL_RECORD |
chia7712
left a comment
chia7712
left a comment
There was a problem hiding this comment.
@hachikuji thanks for your response and updates. Left some questions for this PR.
|
|
||
| private def maybeAddToLocalRemoved(partition: MetadataPartition): Unit = { | ||
| if (partition.isReplicaFor(brokerId)) { | ||
| val currentTopicId = newReverseIdMap.get(partition.topicName) |
There was a problem hiding this comment.
Is it possible that newReverseIdMap has no related id? For example, PartitionRecord is processed before TopicRecord or TopicRecord was discarded (due to error)?
There was a problem hiding this comment.
For another, could it be replaced by prevPartitions.contains(partition.topicName)? It seems all we want to check is the existence of topic name in previous image.
There was a problem hiding this comment.
The intent is to only return the change in _localRemoved if the topic existed in the previous image. If we only check topic name, then successive deletions and recreations might leave some partitions in _localRemoved that were not in the previous image.
It's worth noting that this is strictly more defensive than the current replay logic requires. A new image is built for each batch of records from the controller, and we would never see a topic deleted and recreated (or vice versa) in the same batch. This is an implicit contract though and not protected by the builder API, so I thought we might as well try to make the logic more resilient.
There was a problem hiding this comment.
Thanks for nice explanation. I have another question for this check. It seems to me three collections in prevPartitions should be consistent. For example: a topic which exists one of collection should also exists in other two (vice versa). If above comment is right, why we need this if-else? Calling prevPartitions.contains(partition.topicName) appears to be enough?
|
I am working on some integration tests here. Hopefully then we can wrap this up. |
|
Well, I was going to write some integration tests, but it seems we are still awaiting some infrastructure for that. I tested it out manually and it works correctly. I see the brokers deleting the partition data as expected. I tested deletion as well as recreation. |
chia7712
left a comment
chia7712
left a comment
There was a problem hiding this comment.
@hachikuji thanks for updating code. a couple of comments are left. Otherwise, LGTM.
chia7712
left a comment
chia7712
left a comment
There was a problem hiding this comment.
thanks for this nice patch. LGTM. one trivial question is left.
| import scala.collection.mutable | ||
| import scala.jdk.CollectionConverters._ | ||
|
|
||
| class BrokerMetadataListenerTest { |
There was a problem hiding this comment.
It seems not all messages are covered in this class? If so, is there a jira to complete those test cases?
There was a problem hiding this comment.
Good idea. I filed https://issues.apache.org/jira/browse/KAFKA-12437.
…ed (#10252) This patch implements additional handling logic for `RemoveTopic` records: - Update `MetadataPartitions` to ensure addition of deleted partitions to `localRemoved` set - Ensure topic configs are removed from `ConfigRepository` - Propagate deleted partitions to `GroupCoordinator` so that corresponding offset commits can be removed This patch also changes the controller topic id generation logic to use `Uuid.randomUuid` rather than `Random`. Reviewers: Ismael Juma <ismael@juma.me.uk>, Chia-Ping Tsai <chia7712@gmail.com>
3 participants
This patch implements additional handling logic for
RemoveTopicrecords:MetadataPartitionsto ensure addition of deleted partitions tolocalRemovedsetConfigRepositoryGroupCoordinatorso that corresponding offset commits can be removedThis patch also changes the controller topic id generation logic to use
Uuid.randomUuidrather thanRandom.Committer Checklist (excluded from commit message)