From 86691d2f36a46394edd35fa60b9ed053ebb54b5c Mon Sep 17 00:00:00 2001 From: Zixuan Liu Date: Tue, 21 Dec 2021 13:56:18 +0800 Subject: [PATCH 1/2] [Broker] Remove check resource exists when delete failure domain MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ### Motivation This PR is used to keep same behavior with Pulsar 2.8 and 2.7 version. In Pulsar 2,7 and 2.8, when delete a non-existent failure domain resource, it will return 404 status code. In Pulsar 2.9, when delete a non-existent failure domain resource, it will return 200 status code. Reproduce in Pulsar 2.8: ``` $ docker run -itd \ -p 6650:6650 \ -p 8080:8080 \ apachepulsar/pulsar:2.8.1 \ bin/pulsar standalone $ pulsarctl clusters delete-failure-domain standalone non-existent-failure-domain [✖] code: 404 reason: Domain-name non-existent-failure-domain or cluster standalone does not exist ``` Reproduce in Pulsar 2.9: ``` $ docker run -itd \ -p 6650:6650 \ -p 8080:8080 \ apachepulsar/pulsar:2.9.0 \ bin/pulsar standalone $ pulsarctl clusters delete-failure-domain standalone non-existent-failure-domain Delete failure domain [non-existent-failure-domain] for cluster [standalone] succeed ``` Affected version: 2.9.x ### Modifications - Remove check resource exists when call the delete failure domain API ### Documentation Need to update docs? - [x] `no-need-doc` Signed-off-by: Zixuan Liu --- .../apache/pulsar/broker/resources/ClusterResources.java | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/resources/ClusterResources.java b/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/resources/ClusterResources.java index a4ee27af4929a..42d842e937b1b 100644 --- a/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/resources/ClusterResources.java +++ b/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/resources/ClusterResources.java @@ -121,9 +121,7 @@ public Optional getFailureDomain(String clusterName, String d public void deleteFailureDomain(String clusterName, String domainName) throws MetadataStoreException { String path = joinPath(BASE_CLUSTERS_PATH, clusterName, FAILURE_DOMAIN, domainName); - if (exists(path)) { - delete(path); - } + delete(path); } public void deleteFailureDomains(String clusterName) throws MetadataStoreException { @@ -131,10 +129,7 @@ public void deleteFailureDomains(String clusterName) throws MetadataStoreExcepti for (String domain : getChildren(failureDomainPath)) { delete(joinPath(failureDomainPath, domain)); } - - if (exists(failureDomainPath)) { - delete(failureDomainPath); - } + delete(failureDomainPath); } public void setFailureDomainWithCreate(String clusterName, String domainName, From 56796b4cb219225d111f90681d728a6e0ecc4031 Mon Sep 17 00:00:00 2001 From: Zixuan Liu Date: Fri, 24 Dec 2021 16:26:40 +0800 Subject: [PATCH 2/2] Add test Signed-off-by: Zixuan Liu --- .../broker/resources/ClusterResources.java | 5 + .../broker/admin/AdminApiClusterTest.java | 98 +++++++++++++++++++ 2 files changed, 103 insertions(+) create mode 100644 pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/AdminApiClusterTest.java diff --git a/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/resources/ClusterResources.java b/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/resources/ClusterResources.java index 42d842e937b1b..32586d246926e 100644 --- a/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/resources/ClusterResources.java +++ b/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/resources/ClusterResources.java @@ -126,9 +126,14 @@ public void deleteFailureDomain(String clusterName, String domainName) throws Me public void deleteFailureDomains(String clusterName) throws MetadataStoreException { String failureDomainPath = joinPath(BASE_CLUSTERS_PATH, clusterName, FAILURE_DOMAIN); + if (!exists(failureDomainPath)) { + return; + } + for (String domain : getChildren(failureDomainPath)) { delete(joinPath(failureDomainPath, domain)); } + delete(failureDomainPath); } diff --git a/pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/AdminApiClusterTest.java b/pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/AdminApiClusterTest.java new file mode 100644 index 0000000000000..83ef9af7a2e8f --- /dev/null +++ b/pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/AdminApiClusterTest.java @@ -0,0 +1,98 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.pulsar.broker.admin; + +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertThrows; + +import com.google.common.collect.Sets; +import java.util.UUID; +import lombok.extern.slf4j.Slf4j; +import org.apache.pulsar.broker.auth.MockedPulsarServiceBaseTest; +import org.apache.pulsar.client.admin.PulsarAdminException; +import org.apache.pulsar.common.policies.data.ClusterData; +import org.apache.pulsar.common.policies.data.FailureDomain; +import org.awaitility.Awaitility; +import org.testng.annotations.AfterMethod; +import org.testng.annotations.BeforeMethod; +import org.testng.annotations.Test; + +@Test(groups = "broker") +@Slf4j +public class AdminApiClusterTest extends MockedPulsarServiceBaseTest { + private final String CLUSTER = "test"; + + @BeforeMethod + @Override + public void setup() throws Exception { + resetConfig(); + super.internalSetup(); + admin.clusters() + .createCluster(CLUSTER, ClusterData.builder().serviceUrl(pulsar.getWebServiceAddress()).build()); + } + + @AfterMethod(alwaysRun = true) + @Override + public void cleanup() throws Exception { + super.internalCleanup(); + } + + @Test + public void testDeleteNonExistCluster() { + String cluster = "test-non-exist-cluster-" + UUID.randomUUID(); + + assertThrows(PulsarAdminException.NotFoundException.class, () -> admin.clusters().deleteCluster(cluster)); + } + + @Test + public void testDeleteExistCluster() throws PulsarAdminException { + String cluster = "test-exist-cluster-" + UUID.randomUUID(); + + admin.clusters() + .createCluster(cluster, ClusterData.builder().serviceUrl(pulsar.getWebServiceAddress()).build()); + Awaitility.await().untilAsserted(() -> assertNotNull(admin.clusters().getCluster(cluster))); + + admin.clusters().deleteCluster(cluster); + } + + @Test + public void testDeleteNonExistentFailureDomain() { + assertThrows(PulsarAdminException.NotFoundException.class, + () -> admin.clusters().deleteFailureDomain(CLUSTER, "non-existent-failure-domain")); + } + + @Test + public void testDeleteNonExistentFailureDomainInNonExistCluster() { + assertThrows(PulsarAdminException.PreconditionFailedException.class, + () -> admin.clusters().deleteFailureDomain(CLUSTER + UUID.randomUUID(), + "non-existent-failure-domain")); + } + + @Test + public void testDeleteExistFailureDomain() throws PulsarAdminException { + String domainName = CLUSTER + "-failure-domain"; + FailureDomain domain = FailureDomain.builder() + .brokers(Sets.newHashSet("b1", "b2", "b3")) + .build(); + admin.clusters().createFailureDomain(CLUSTER, domainName, domain); + Awaitility.await().untilAsserted(() -> admin.clusters().getFailureDomain(CLUSTER, domainName)); + + admin.clusters().deleteFailureDomain(CLUSTER, domainName); + } +}