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

[fix][admin] Fix delete tenant #19925

Merged
merged 2 commits into from
Mar 27, 2023
Merged

[fix][admin] Fix delete tenant #19925

merged 2 commits into from
Mar 27, 2023

Conversation

nodece
Copy link
Member

@nodece nodece commented Mar 25, 2023 •
edited
Loading

Fixes #19921

Motivation

Failed to delete tenant, see log:

2023-03-25T18:40:15,174+0800 [pulsar-web-48-12] ERROR org.apache.pulsar.broker.admin.impl.TenantsBase - [null] Failed to delete tenant newtenant
org.apache.pulsar.metadata.api.MetadataStoreException$NotFoundException: path /admin/local-policies/newtenant not found.
        at org.apache.pulsar.metadata.impl.RocksdbMetadataStore.storeDelete(RocksdbMetadataStore.java:480) ~[pulsar-metadata.jar:3.0.0-SNAPSHOT]
        at org.apache.pulsar.metadata.impl.AbstractMetadataStore.deleteInternal(AbstractMetadataStore.java:386) ~[pulsar-metadata.jar:3.0.0-SNAPSHOT]
        at org.apache.pulsar.metadata.impl.AbstractMetadataStore.delete(AbstractMetadataStore.java:373) ~[pulsar-metadata.jar:3.0.0-SNAPSHOT]
        at org.apache.pulsar.metadata.cache.impl.MetadataCacheImpl.delete(MetadataCacheImpl.java:248) ~[pulsar-metadata.jar:3.0.0-SNAPSHOT]
        at org.apache.pulsar.broker.resources.BaseResources.deleteAsync(BaseResources.java:160) ~[pulsar-broker-common.jar:3.0.0-SNAPSHOT]
        at org.apache.pulsar.broker.resources.LocalPoliciesResources.deleteLocalPoliciesTenantAsync(LocalPoliciesResources.java:88) ~[pulsar-broker-common.jar:3.0.0-SNAPSHOT]
        at org.apache.pulsar.broker.admin.impl.TenantsBase.lambda$internalDeleteTenantAsync$33(TenantsBase.java:238) ~[pulsar-broker.jar:3.0.0-SNAPSHOT]

When deleting a tenant, we delete a path that does not exist, so throw an exception.

Modifications

  • Check whether the /admin/local-policies/{tenant} exists, and then delete it

Verifying this change

  • Make sure that the change passes the CI checks.
    Added AdminApiTenantTest.

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

@nodece nodece requested a review from tisonkun March 25, 2023 11:51
@nodece nodece self-assigned this Mar 25, 2023
@nodece nodece added type/bug The PR fixed a bug or issue reported a bug area/admin labels Mar 25, 2023
@nodece nodece added this to the 3.0.0 milestone Mar 25, 2023
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Mar 25, 2023
@nodece
Copy link
Member Author

nodece commented Mar 25, 2023

/pulsarbot rerun-failure-checks

@nodece nodece changed the title [fix][admin]: Fix delete tenant [fix][admin] Fix delete tenant Mar 25, 2023
@nodece nodece mentioned this pull request Mar 25, 2023
Closed
2 tasks
tisonkun
tisonkun approved these changes Mar 25, 2023
View reviewed changes
Copy link
Member

@tisonkun tisonkun left a comment

Choose a reason for hiding this comment

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

Locally verified. LGTM.

Comments inline.

pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/AdminApiTenantTest.java Outdated
import java.util.List;
import java.util.UUID;

import static org.testng.Assert.*;
Copy link
Member

Choose a reason for hiding this comment

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

Expand star import and sort by:

pulsar/src/idea-code-style.xml

Lines 30 to 31 in 329e80b

<package name="" withSubpackages="true" static="true" />
<package name="" withSubpackages="true" static="false" />

?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, I should use the static import.

pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/AdminApiTenantTest.java Show resolved Hide resolved
@nodece
Fix import
aafd7e9 
Signed-off-by: nodece <[email protected]>
@nodece
Copy link
Member Author

nodece commented Mar 26, 2023

/pulsarbot rerun-failure-checks

@tisonkun
Copy link
Member

Merging...

@nodece Thanks for preparing this patch :)

@tisonkun tisonkun merged commit 32ad906 into apache:master Mar 27, 2023
nodece added a commit that referenced this pull request Mar 27, 2023
@nodece
[fix][admin] Delete tenant local policy only if exist (#19925)
8d6afba 
Signed-off-by: nodece <[email protected]>
(cherry picked from commit 32ad906)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tisonkun tisonkun tisonkun approved these changes

@michaeljmarshall michaeljmarshall Awaiting requested review from michaeljmarshall

Assignees

@nodece nodece

Labels
area/admin doc-not-needed Your PR changes do not impact docs ready-to-test type/bug The PR fixed a bug or issue reported a bug
Projects
None yet
Milestone
3.0.0
Development

Successfully merging this pull request may close these issues.

[Bug] Deleting a tenant results in a HTTP 500 error, but deletes the tenant
2 participants
@nodece @tisonkun