You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2021/02/23 03:17:05 UTC
[GitHub] [pulsar] murong00 opened a new pull request #9677: [pulsar-broker] Added support to force deleting tenant
murong00 opened a new pull request #9677:
URL: https://github.com/apache/pulsar/pull/9677
### 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] murong00 commented on a change in pull request #9677: [pulsar-broker] Added support to force deleting tenant
Posted by GitBox <gi...@apache.org>.
murong00 commented on a change in pull request #9677:
URL: https://github.com/apache/pulsar/pull/9677#discussion_r581832514
##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/AdminApiTest.java
##########
@@ -1108,6 +1108,46 @@ public void testDeleteNamespaceBundle(Integer numBundles) throws Exception {
assertEquals(admin.namespaces().getNamespaces("prop-xyz", "test"), Lists.newArrayList());
}
+ @Test
+ public void testDeleteTenantForcefully() throws Exception {
+ String tenant = "my-tenant";
+ assertFalse(admin.tenants().getTenants().contains(tenant));
+
+ // create tenant
+ admin.tenants().createTenant(tenant,
+ new TenantInfo(Sets.newHashSet("role1", "role2"), Sets.newHashSet("test")));
+
+ assertTrue(admin.tenants().getTenants().contains(tenant));
+
+ // create namespace
+ String namespace = tenant + "/my-ns";
+ admin.namespaces().createNamespace("my-tenant/my-ns", Sets.newHashSet("test"));
+
+ assertEquals(admin.namespaces().getNamespaces(tenant), Lists.newArrayList("my-tenant/my-ns"));
+
+ // create topic
+ String topic = namespace + "/my-topic";
+ admin.topics().createPartitionedTopic(topic, 10);
+
+ assertFalse(admin.topics().getList(namespace).isEmpty());
+
+ try {
+ admin.tenants().deleteTenant(tenant, false);
+ fail("should have failed");
+ } catch (PulsarAdminException e) {
+ // Expected: cannot delete non-empty tenant
+ }
+
+ //
Review comment:
Just want to comment "delete tenant forcefully", sorry for the omission.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] murong00 commented on pull request #9677: [pulsar-broker] Added support to force deleting tenant
Posted by GitBox <gi...@apache.org>.
murong00 commented on pull request #9677:
URL: https://github.com/apache/pulsar/pull/9677#issuecomment-786468378
/pulsarbot run-failure-checks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] murong00 commented on a change in pull request #9677: [pulsar-broker] Added support to force deleting tenant
Posted by GitBox <gi...@apache.org>.
murong00 commented on a change in pull request #9677:
URL: https://github.com/apache/pulsar/pull/9677#discussion_r581834411
##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/AdminApiTest.java
##########
@@ -1108,6 +1108,46 @@ public void testDeleteNamespaceBundle(Integer numBundles) throws Exception {
assertEquals(admin.namespaces().getNamespaces("prop-xyz", "test"), Lists.newArrayList());
}
+ @Test
+ public void testDeleteTenantForcefully() throws Exception {
+ String tenant = "my-tenant";
+ assertFalse(admin.tenants().getTenants().contains(tenant));
+
+ // create tenant
+ admin.tenants().createTenant(tenant,
+ new TenantInfo(Sets.newHashSet("role1", "role2"), Sets.newHashSet("test")));
+
+ assertTrue(admin.tenants().getTenants().contains(tenant));
+
+ // create namespace
+ String namespace = tenant + "/my-ns";
+ admin.namespaces().createNamespace("my-tenant/my-ns", Sets.newHashSet("test"));
+
+ assertEquals(admin.namespaces().getNamespaces(tenant), Lists.newArrayList("my-tenant/my-ns"));
+
+ // create topic
+ String topic = namespace + "/my-topic";
+ admin.topics().createPartitionedTopic(topic, 10);
+
+ assertFalse(admin.topics().getList(namespace).isEmpty());
+
+ try {
+ admin.tenants().deleteTenant(tenant, false);
+ fail("should have failed");
+ } catch (PulsarAdminException e) {
+ // Expected: cannot delete non-empty tenant
+ }
+
+ //
Review comment:
Done.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] murong00 commented on pull request #9677: [pulsar-broker] Added support to force deleting tenant
Posted by GitBox <gi...@apache.org>.
murong00 commented on pull request #9677:
URL: https://github.com/apache/pulsar/pull/9677#issuecomment-785501060
/pulsarbot run-failure-checks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Anonymitaet commented on a change in pull request #9677: [pulsar-broker] Added support to force deleting tenant
Posted by GitBox <gi...@apache.org>.
Anonymitaet commented on a change in pull request #9677:
URL: https://github.com/apache/pulsar/pull/9677#discussion_r582566811
##########
File path: site2/docs/reference-pulsar-admin.md
##########
@@ -2451,6 +2451,11 @@ Usage
$ pulsar-admin tenants delete tenant-name
```
+Options
+|Flag|Description|Default|
+|----|---|---|
+|`-f`, `--force`|Delete tenant forcefully by force deleting all namespaces under it|false|
Review comment:
```suggestion
|`-f`, `--force`|Delete a tenant forcefully by deleting all namespaces under it. |false|
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui merged pull request #9677: [pulsar-broker] Added support to force deleting tenant
Posted by GitBox <gi...@apache.org>.
codelipenghui merged pull request #9677:
URL: https://github.com/apache/pulsar/pull/9677
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] murong00 commented on pull request #9677: [pulsar-broker] Added support to force deleting tenant
Posted by GitBox <gi...@apache.org>.
murong00 commented on pull request #9677:
URL: https://github.com/apache/pulsar/pull/9677#issuecomment-790205447
> @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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] murong00 commented on a change in pull request #9677: [pulsar-broker] Added support to force deleting tenant
Posted by GitBox <gi...@apache.org>.
murong00 commented on a change in pull request #9677:
URL: https://github.com/apache/pulsar/pull/9677#discussion_r582578339
##########
File path: site2/docs/reference-pulsar-admin.md
##########
@@ -2451,6 +2451,11 @@ Usage
$ pulsar-admin tenants delete tenant-name
```
+Options
+|Flag|Description|Default|
+|----|---|---|
+|`-f`, `--force`|Delete tenant forcefully by force deleting all namespaces under it|false|
Review comment:
Done.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] jiazhai commented on pull request #9677: [pulsar-broker] Added support to force deleting tenant
Posted by GitBox <gi...@apache.org>.
jiazhai commented on pull request #9677:
URL: https://github.com/apache/pulsar/pull/9677#issuecomment-784294550
@BewareMyPower Would you please also take a look of this PR?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] murong00 commented on pull request #9677: [pulsar-broker] Added support to force deleting tenant
Posted by GitBox <gi...@apache.org>.
murong00 commented on pull request #9677:
URL: https://github.com/apache/pulsar/pull/9677#issuecomment-785710950
/pulsarbot run-failure-checks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] murong00 removed a comment on pull request #9677: [pulsar-broker] Added support to force deleting tenant
Posted by GitBox <gi...@apache.org>.
murong00 removed a comment on pull request #9677:
URL: https://github.com/apache/pulsar/pull/9677#issuecomment-786336226
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] murong00 commented on pull request #9677: [pulsar-broker] Added support to force deleting tenant
Posted by GitBox <gi...@apache.org>.
murong00 commented on pull request #9677:
URL: https://github.com/apache/pulsar/pull/9677#issuecomment-786336226
/pulsarbot run-failure-checks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on pull request #9677: [pulsar-broker] Added support to force deleting tenant
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on pull request #9677:
URL: https://github.com/apache/pulsar/pull/9677#issuecomment-790189060
@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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] murong00 commented on pull request #9677: [pulsar-broker] Added support to force deleting tenant
Posted by GitBox <gi...@apache.org>.
murong00 commented on pull request #9677:
URL: https://github.com/apache/pulsar/pull/9677#issuecomment-785635939
> @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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Anonymitaet commented on pull request #9677: [pulsar-broker] Added support to force deleting tenant
Posted by GitBox <gi...@apache.org>.
Anonymitaet commented on pull request #9677:
URL: https://github.com/apache/pulsar/pull/9677#issuecomment-785553909
@murong00 thanks for your coding work, would you like to add docs for this change? I'd like to help review. Thanks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] murong00 commented on pull request #9677: [pulsar-broker] Added support to force deleting tenant
Posted by GitBox <gi...@apache.org>.
murong00 commented on pull request #9677:
URL: https://github.com/apache/pulsar/pull/9677#issuecomment-787825350
@codelipenghui Please help to take a look about this pr, thanks.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] murong00 commented on pull request #9677: [pulsar-broker] Added support to force deleting tenant
Posted by GitBox <gi...@apache.org>.
murong00 commented on pull request #9677:
URL: https://github.com/apache/pulsar/pull/9677#issuecomment-786435846
/pulsarbot run-failure-checks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Anonymitaet edited a comment on pull request #9677: [pulsar-broker] Added support to force deleting tenant
Posted by GitBox <gi...@apache.org>.
Anonymitaet edited a comment on pull request #9677:
URL: https://github.com/apache/pulsar/pull/9677#issuecomment-785553909
@murong00 thanks for your coding work.
1. Does this apply to 2.7.1?
2. Would you like to add docs for this change?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on a change in pull request #9677: [pulsar-broker] Added support to force deleting tenant
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on a change in pull request #9677:
URL: https://github.com/apache/pulsar/pull/9677#discussion_r581790586
##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/AdminApiTest.java
##########
@@ -1108,6 +1108,46 @@ public void testDeleteNamespaceBundle(Integer numBundles) throws Exception {
assertEquals(admin.namespaces().getNamespaces("prop-xyz", "test"), Lists.newArrayList());
}
+ @Test
+ public void testDeleteTenantForcefully() throws Exception {
+ String tenant = "my-tenant";
+ assertFalse(admin.tenants().getTenants().contains(tenant));
+
+ // create tenant
+ admin.tenants().createTenant(tenant,
+ new TenantInfo(Sets.newHashSet("role1", "role2"), Sets.newHashSet("test")));
+
+ assertTrue(admin.tenants().getTenants().contains(tenant));
+
+ // create namespace
+ String namespace = tenant + "/my-ns";
+ admin.namespaces().createNamespace("my-tenant/my-ns", Sets.newHashSet("test"));
+
+ assertEquals(admin.namespaces().getNamespaces(tenant), Lists.newArrayList("my-tenant/my-ns"));
+
+ // create topic
+ String topic = namespace + "/my-topic";
+ admin.topics().createPartitionedTopic(topic, 10);
+
+ assertFalse(admin.topics().getList(namespace).isEmpty());
+
+ try {
+ admin.tenants().deleteTenant(tenant, false);
+ fail("should have failed");
+ } catch (PulsarAdminException e) {
+ // Expected: cannot delete non-empty tenant
+ }
+
+ //
Review comment:
What is your intention for this comment?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Anonymitaet edited a comment on pull request #9677: [pulsar-broker] Added support to force deleting tenant
Posted by GitBox <gi...@apache.org>.
Anonymitaet edited a comment on pull request #9677:
URL: https://github.com/apache/pulsar/pull/9677#issuecomment-785553909
@murong00 thanks for your coding work.
1. Would you like to add docs for this change? I'd like to help review. Thanks
2. Does this apply to 2.7.1?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on pull request #9677: [pulsar-broker] Added support to force deleting tenant
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on pull request #9677:
URL: https://github.com/apache/pulsar/pull/9677#issuecomment-790219368
@murong00 Ok, I will create an issue first
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Anonymitaet edited a comment on pull request #9677: [pulsar-broker] Added support to force deleting tenant
Posted by GitBox <gi...@apache.org>.
Anonymitaet edited a comment on pull request #9677:
URL: https://github.com/apache/pulsar/pull/9677#issuecomment-785553909
@murong00 thanks for your coding work.
1. Does this apply to 2.7.1?
2. Would you like to add docs for this change? I'd like to help review. Thanks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org