Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Enhancement] avoid duplicate deletion of schema #11640

Conversation

wuzhanpeng
Copy link
Contributor

Motivation

Currently when I need to delete a namespace forcedly, the NamespacesBase#internalDeleteNamespaceForcefully will firstly list all the topics under the namespace and then delete them concurrently without distinguishing wheather the topic is partitioned or non-partitioned. This behaivor will cause duplicate deletion of schema when the topic list contains partitioned topics, because all the partitions of the partitioned topic share the same schema.

Modifications

Distinguish partitioned topic to avoid duplicate deletion of the same schema.

Does this pull request potentially affect one of the following parts:

If yes was chosen, please highlight the changes

  • Dependencies (does it add or upgrade a dependency): no
  • The public API: no
  • The schema: no
  • The default values of configurations: no
  • The wire protocol: no
  • The rest endpoints: no
  • The admin cli options: no
  • Anything that affects deployment: no

Documentation

This PR does not need to update the docs.

@eolivelli
Copy link
Contributor

I believe that this is a good idea.

Can you please add a test case ?

@hangc0276
Copy link
Contributor

Nice catch!
Would you please add the test?

@hangc0276 hangc0276 added release/2.8.2 type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages labels Aug 11, 2021
@hangc0276 hangc0276 added this to the 2.9.0 milestone Aug 11, 2021
@Anonymitaet Anonymitaet added the doc-not-needed Your PR changes do not impact docs label Aug 12, 2021
@Anonymitaet
Copy link
Member

@hangc0276 thanks for triaging PR and issues. When you label PRs, if you find some PRs do not need to update doc, could you please help label the PR with no-need-doc in the future? Thanks

@hangc0276
Copy link
Contributor

@hangc0276 thanks for triaging PR and issues. When you label PRs, if you find some PRs do not need to update doc, could you please help label the PR with no-need-doc in the future? Thanks

@Anonymitaet Ok, I will add doc label in the following PRs, thank you!

Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

Good work
Can you add a test case?

@wuzhanpeng
Copy link
Contributor Author

Good work
Can you add a test case?

Sorry for the late reply! I will add a test later.

@wuzhanpeng
Copy link
Contributor Author

/pulsarbot run-failure-checks

2 similar comments
@wuzhanpeng
Copy link
Contributor Author

/pulsarbot run-failure-checks

@wuzhanpeng
Copy link
Contributor Author

/pulsarbot run-failure-checks

@wuzhanpeng
Copy link
Contributor Author

/pulsarbot run-failure-checks

@wuzhanpeng
Copy link
Contributor Author

@eolivelli @hangc0276 Sorry for the late. I supplemented a test case. PTAL~

final String exNp = "persistent://" + exNs + "/np";
admin.topics().createNonPartitionedTopic(exNp);
// insert an invalid topic name
pulsar.getLocalMetadataStore().put(
Copy link
Contributor

Choose a reason for hiding this comment

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

why this is related to this patch ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are actually three scenarios I tested for partitioned topics, non-partitioned topics and exceptions while handling internal deletion.

Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

LGTM

}
}

log.info("Successfully send deletion command of partitioned-topics:{} "
Copy link
Contributor

Choose a reason for hiding this comment

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

It should be a debug level log? If we have many partitioned topics and non-partitioned-topics, we will get a very long log here, maybe print the topic count is better?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your suggestion! I have addressed the comment. PTAL~ @codelipenghui

@codelipenghui codelipenghui merged commit e28f69e into apache:master Aug 21, 2021
codelipenghui pushed a commit that referenced this pull request Sep 9, 2021
### Motivation

Currently when I need to delete a namespace forcedly, the `NamespacesBase#internalDeleteNamespaceForcefully` will firstly list all the topics under the namespace and then delete them concurrently without distinguishing wheather the topic is partitioned or non-partitioned. This behaivor will cause duplicate deletion of schema when the topic list contains partitioned topics, because all the partitions of the partitioned topic share the same schema.

(cherry picked from commit e28f69e)
@codelipenghui codelipenghui added the cherry-picked/branch-2.8 Archived: 2.8 is end of life label Sep 9, 2021
bharanic-dev pushed a commit to bharanic-dev/pulsar that referenced this pull request Mar 18, 2022
### Motivation

Currently when I need to delete a namespace forcedly, the `NamespacesBase#internalDeleteNamespaceForcefully` will firstly list all the topics under the namespace and then delete them concurrently without distinguishing wheather the topic is partitioned or non-partitioned. This behaivor will cause duplicate deletion of schema when the topic list contains partitioned topics, because all the partitions of the partitioned topic share the same schema.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-picked/branch-2.8 Archived: 2.8 is end of life doc-not-needed Your PR changes do not impact docs release/2.8.2 type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants