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.

Sign up for GitHub

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

Jump to bottom

[pulsar-broker] Added support to force deleting tenant #9677

Merged
merged 5 commits into from
Mar 4, 2021
Merged

[pulsar-broker] Added support to force deleting tenant #9677

merged 5 commits into from
Mar 4, 2021

Conversation

murong00
Copy link
Contributor

Motivation

Some users expect an option to force deleting tenant for simplicity, it can be useful in some situations.

Modifications

Add a optional field to force the deletion of all stuffs related to tenant and a related unit test.

@codelipenghui codelipenghui added doc-required Your PR changes impact docs and you will update later. area/admin labels Feb 23, 2021
@codelipenghui codelipenghui added this to the 2.8.0 milestone Feb 23, 2021
@jiazhai
Copy link
Member

jiazhai commented Feb 23, 2021

@BewareMyPower Would you please also take a look of this PR?

BewareMyPower
BewareMyPower reviewed Feb 24, 2021
View reviewed changes
pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/AdminApiTest.java Outdated
// Expected: cannot delete non-empty tenant
}

//
Copy link
Contributor

Choose a reason for hiding this comment

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

What is your intention for this comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just want to comment "delete tenant forcefully", sorry for the omission.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

BewareMyPower
BewareMyPower approved these changes Feb 24, 2021
View reviewed changes
Copy link
Contributor

@BewareMyPower BewareMyPower left a comment

Choose a reason for hiding this comment

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

LGTM

@murong00
Copy link
Contributor Author

/pulsarbot run-failure-checks

@Anonymitaet
Copy link
Member

Anonymitaet commented Feb 25, 2021 •
edited
Loading

@murong00 thanks for your coding work.

  1. Does this apply to 2.7.1?

  2. Would you like to add docs for this change?

@murong00
Copy link
Contributor Author

@murong00 thanks for your coding work.

  1. Does this apply to 2.7.1?
  2. Would you like to add docs for this change?

@Anonymitaet

  1. Applying this to 2.8.0 seems more appropriate since it is a new feature.
  2. Done, please help to take a look.

Anonymitaet
Anonymitaet reviewed Feb 25, 2021
View reviewed changes
Copy link
Member

@Anonymitaet Anonymitaet left a comment

Choose a reason for hiding this comment

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

Thanks. Please update the description for all occurrences.

site2/docs/reference-pulsar-admin.md Outdated
Options
|Flag|Description|Default|
|----|---|---|
|`-f`, `--force`|Delete tenant forcefully by force deleting all namespaces under it|false|
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
|`-f`, `--force`|Delete tenant forcefully by force deleting all namespaces under it|false|
|`-f`, `--force`|Delete a tenant forcefully by deleting all namespaces under it. |false|

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@murong00
Copy link
Contributor Author

/pulsarbot run-failure-checks

@murong00
Copy link
Contributor Author

/pulsarbot run-failure-checks

@murong00
Copy link
Contributor Author

murong00 commented Mar 1, 2021

@codelipenghui Please help to take a look about this pr, thanks.

@codelipenghui
Copy link
Contributor

@murong00 Then change looks good to me, shall we need to control the force delete tenants or namespaces in the broker.conf? such as forceDeleteNamepaceAllowed, forceDeleteTenantAllowed? This will help the Pulsar maintainer to maintain the cluster as expected. If this makes sense, we can create an issue or send a PR directly.

I will merge this PR first.

@codelipenghui codelipenghui merged commit a5e795c into apache:master Mar 4, 2021
@Anonymitaet Anonymitaet added doc Your PR contains doc changes, no matter whether the changes are in markdown or code files. and removed doc-required Your PR changes impact docs and you will update later. labels Mar 4, 2021
@murong00
Copy link
Contributor Author

murong00 commented Mar 4, 2021

@murong00 Then change looks good to me, shall we need to control the force delete tenants or namespaces in the broker.conf? such as forceDeleteNamepaceAllowed, forceDeleteTenantAllowed? This will help the Pulsar maintainer to maintain the cluster as expected. If this makes sense, we can create an issue or send a PR directly.

That sounds like a good idea, I will accomplish this when available.

@codelipenghui
Copy link
Contributor

@murong00 Ok, I will create an issue first

@codelipenghui codelipenghui mentioned this pull request Mar 4, 2021
Closed
@sijie sijie mentioned this pull request Mar 4, 2021
Closed
mlyahmed pushed a commit to mlyahmed/pulsar that referenced this pull request Mar 5, 2021
@murong00 @mlyahmed
[pulsar-broker] Added support to force deleting tenant (apache#9677)
418aabd 
### Motivation

Some users expect an option to force deleting tenant for simplicity, it can be useful in some situations.

### Modifications

Add a optional field to force the deletion of all stuffs related to tenant and a related unit test.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@codelipenghui codelipenghui codelipenghui approved these changes

@BewareMyPower BewareMyPower BewareMyPower approved these changes

@Anonymitaet Anonymitaet Anonymitaet approved these changes

@rdhabalia rdhabalia Awaiting requested review from rdhabalia

@sijie sijie Awaiting requested review from sijie

@315157973 315157973 Awaiting requested review from 315157973

@hangc0276 hangc0276 Awaiting requested review from hangc0276

@jiazhai jiazhai Awaiting requested review from jiazhai

@zymap zymap Awaiting requested review from zymap

Assignees

@murong00 murong00

Labels
area/admin doc Your PR contains doc changes, no matter whether the changes are in markdown or code files.
Projects
None yet
Milestone
2.8.0
Development

Successfully merging this pull request may close these issues.
5 participants
@murong00 @jiazhai @Anonymitaet @codelipenghui @BewareMyPower