Skip to content

KAFKA-14337: correctly remove topicsWithCollisionChars after topic deletion#12790

Merged
hachikuji merged 1 commit intoapache:trunkfrom
showuon:KAFKA-14337
Oct 28, 2022
Merged

KAFKA-14337: correctly remove topicsWithCollisionChars after topic deletion#12790
hachikuji merged 1 commit intoapache:trunkfrom
showuon:KAFKA-14337

Conversation

@showuon
Copy link
Copy Markdown
Member

@showuon showuon commented Oct 26, 2022

In #11910 , we added a feature to prevent topics with conflicting metrics names from being created. We added a map to store the normalized topic name to the topic names, but we didn't remove it correctly while deleting topics. This PR fixes this bug and add a test.

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The key for topicsWithCollisionChars should be normalized name, not original name.

@showuon
Copy link
Copy Markdown
Member Author

showuon commented Oct 26, 2022

@dengziming @cmccabe , please take a look. Thanks.

Copy link
Copy Markdown
Member

@soarez soarez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@dengziming dengziming self-assigned this Oct 27, 2022
Copy link
Copy Markdown
Member

@dengziming dengziming left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch @showuon , can we also add a test in ReplicationControlManagerTest?

TestUtils.verifyTopicDeletion(zkClientOrNull, topicWithCollidingChar, 1, brokers)

val createTopic: Executable = () => createAndWaitTopic(createOpts)
assertDoesNotThrow(createTopic)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm investigating why there will be compile error when directly using assertDoesNotThrow(() => createAndWaitTopic(createOpts)).

@hachikuji
Copy link
Copy Markdown
Contributor

Great find! Agree with @dengziming that we should have a test in ReplicationControlManagerTest. Otherwise, LGTM.

Copy link
Copy Markdown
Contributor

@hachikuji hachikuji left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the fix is straightforward, I'm inclined to check in as is. I'm a tad anxious to get this patch since we are trying to bring kraft to production. I've opened a separate PR with a unit test for ReplicationControlManager: #12796.

@hachikuji hachikuji merged commit c1bb307 into apache:trunk Oct 28, 2022
hachikuji pushed a commit that referenced this pull request Oct 28, 2022
…letion (#12790)

In #11910 , we added a feature to prevent topics with conflicting metrics names from being created. We added a map to store the normalized topic name to the topic names, but we didn't remove it correctly while deleting topics. This PR fixes this bug and add a test.

Reviewers: Igor Soarez <i@soarez.me>, dengziming <dengziming1993@gmail.com>, Jason Gustafson <jason@confluent.io>
hachikuji added a commit that referenced this pull request Oct 29, 2022
This patch adds a unit test for topic recreation with colliding characters (such as `.`). This was broken up until #12790. 

Reviewers: José Armando García Sancio <jsancio@users.noreply.github.com>
guozhangwang pushed a commit to guozhangwang/kafka that referenced this pull request Jan 25, 2023
…letion (apache#12790)

In apache#11910 , we added a feature to prevent topics with conflicting metrics names from being created. We added a map to store the normalized topic name to the topic names, but we didn't remove it correctly while deleting topics. This PR fixes this bug and add a test.

Reviewers: Igor Soarez <i@soarez.me>, dengziming <dengziming1993@gmail.com>, Jason Gustafson <jason@confluent.io>
guozhangwang pushed a commit to guozhangwang/kafka that referenced this pull request Jan 25, 2023
…e#12796)

This patch adds a unit test for topic recreation with colliding characters (such as `.`). This was broken up until apache#12790. 

Reviewers: José Armando García Sancio <jsancio@users.noreply.github.com>
rutvijmehta-harness pushed a commit to rutvijmehta-harness/kafka that referenced this pull request Feb 9, 2024
…letion (apache#12790)

In apache#11910 , we added a feature to prevent topics with conflicting metrics names from being created. We added a map to store the normalized topic name to the topic names, but we didn't remove it correctly while deleting topics. This PR fixes this bug and add a test.

Reviewers: Igor Soarez <i@soarez.me>, dengziming <dengziming1993@gmail.com>, Jason Gustafson <jason@confluent.io>
rutvijmehta-harness added a commit to rutvijmehta-harness/kafka that referenced this pull request Feb 9, 2024
…letion (apache#12790) (#90)

In apache#11910 , we added a feature to prevent topics with conflicting metrics names from being created. We added a map to store the normalized topic name to the topic names, but we didn't remove it correctly while deleting topics. This PR fixes this bug and add a test.

Reviewers: Igor Soarez <i@soarez.me>, dengziming <dengziming1993@gmail.com>, Jason Gustafson <jason@confluent.io>

Co-authored-by: Luke Chen <showuon@gmail.com>
rutvijmehta-harness pushed a commit to rutvijmehta-harness/kafka that referenced this pull request Feb 9, 2024
…e#12796)

This patch adds a unit test for topic recreation with colliding characters (such as `.`). This was broken up until apache#12790. 

Reviewers: José Armando García Sancio <jsancio@users.noreply.github.com>
rutvijmehta-harness added a commit to rutvijmehta-harness/kafka that referenced this pull request Feb 9, 2024
…e#12796) (#91)

This patch adds a unit test for topic recreation with colliding characters (such as `.`). This was broken up until apache#12790. 

Reviewers: José Armando García Sancio <jsancio@users.noreply.github.com>

Co-authored-by: Jason Gustafson <jason@confluent.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants