You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by pe...@apache.org on 2020/10/26 08:16:15 UTC
[pulsar] branch master updated: [Issue 8346][Admin API] Validate
retention policy (#8358)
This is an automated email from the ASF dual-hosted git repository.
penghui pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/pulsar.git
The following commit(s) were added to refs/heads/master by this push:
new db5320d [Issue 8346][Admin API] Validate retention policy (#8358)
db5320d is described below
commit db5320df3470f89f6790340e888019c4f6e12983
Author: Lari Hotari <lh...@users.noreply.github.com>
AuthorDate: Mon Oct 26 10:16:00 2020 +0200
[Issue 8346][Admin API] Validate retention policy (#8358)
Fixes #8346
### Motivation
See #8346, #8345
### Modifications
Prevent setting invalid retention policy where either size or time limit is set to zero while the other limit has a non-zero value.
The reason for this is that setting either size or time limit to zero will effectively disable the retention policy.
It is confusing for the end-user if it's possible to set a value for the other limit while it's ignored when the other limit has
the value of zero. This might lead to the incorrect assumption that the value of 0 has the meaning that the limit isn't effective.
The validation will provide the Admin API end user a useful error message if the validation fails.
---
.../pulsar/broker/admin/impl/NamespacesBase.java | 16 +++++
.../apache/pulsar/broker/admin/NamespacesTest.java | 77 +++++++++++++++++++++-
.../broker/service/PersistentTopicE2ETest.java | 8 +--
3 files changed, 96 insertions(+), 5 deletions(-)
diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/NamespacesBase.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/NamespacesBase.java
index 2bdd04c..df55992 100644
--- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/NamespacesBase.java
+++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/NamespacesBase.java
@@ -1753,6 +1753,7 @@ public abstract class NamespacesBase extends AdminResource {
}
protected void internalSetRetention(RetentionPolicies retention) {
+ validateRetentionPolicies(retention);
validateNamespacePolicyOperation(namespaceName, PolicyName.RETENTION, PolicyOperation.WRITE);
validatePoliciesReadOnlyAccess();
@@ -2480,8 +2481,23 @@ public abstract class NamespacesBase extends AdminResource {
if (policies.persistence != null) {
validatePersistencePolicies(policies.persistence);
}
+
+ if (policies.retention_policies != null) {
+ validateRetentionPolicies(policies.retention_policies);
+ }
}
+ protected void validateRetentionPolicies(RetentionPolicies retention) {
+ checkArgument(retention.getRetentionSizeInMB() >= -1,
+ "Invalid retention policy: size limit must be >= -1");
+ checkArgument(retention.getRetentionTimeInMinutes() >= -1,
+ "Invalid retention policy: time limit must be >= -1");
+ checkArgument((retention.getRetentionTimeInMinutes() != 0 && retention.getRetentionSizeInMB() != 0) ||
+ (retention.getRetentionTimeInMinutes() == 0 && retention.getRetentionSizeInMB() == 0),
+ "Invalid retention policy: Setting a single time or size limit to 0 is invalid when " +
+ "one of the limits has a non-zero value. Use the value of -1 instead of 0 to ignore a " +
+ "specific limit. To disable retention both limits must be set to 0.");
+ }
protected int internalGetMaxProducersPerTopic() {
validateNamespacePolicyOperation(namespaceName, PolicyName.MAX_PRODUCERS, PolicyOperation.READ);
diff --git a/pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/NamespacesTest.java b/pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/NamespacesTest.java
index d4d7f88..6509ff9 100644
--- a/pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/NamespacesTest.java
+++ b/pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/NamespacesTest.java
@@ -48,6 +48,7 @@ import java.util.Set;
import java.util.UUID;
import java.util.concurrent.CompletableFuture;
import java.util.concurrent.ConcurrentHashMap;
+import javax.ws.rs.BadRequestException;
import javax.ws.rs.ClientErrorException;
import javax.ws.rs.WebApplicationException;
import javax.ws.rs.container.AsyncResponse;
@@ -1356,4 +1357,78 @@ public class NamespacesTest extends MockedPulsarServiceBaseTest {
// Expected
}
}
-}
\ No newline at end of file
+
+ @Test
+ public void testRetentionPolicyValidation() throws Exception {
+ String namespace = this.testTenant + "/namespace-" + System.nanoTime();
+
+ admin.namespaces().createNamespace(namespace, Sets.newHashSet(testLocalCluster));
+
+ // should pass
+ admin.namespaces().setRetention(namespace, new RetentionPolicies());
+ admin.namespaces().setRetention(namespace, new RetentionPolicies(-1, -1));
+ admin.namespaces().setRetention(namespace, new RetentionPolicies(1, 1));
+
+ // should not pass validation
+ assertInvalidRetentionPolicy(namespace, 1, 0);
+ assertInvalidRetentionPolicy(namespace, 0, 1);
+ assertInvalidRetentionPolicy(namespace, -1, 0);
+ assertInvalidRetentionPolicy(namespace, 0, -1);
+ assertInvalidRetentionPolicy(namespace, -2, 1);
+ assertInvalidRetentionPolicy(namespace, 1, -2);
+
+ admin.namespaces().deleteNamespace(namespace);
+ }
+
+ private void assertInvalidRetentionPolicy(String namespace, int retentionTimeInMinutes, int retentionSizeInMB) {
+ try {
+ RetentionPolicies retention = new RetentionPolicies(retentionTimeInMinutes, retentionSizeInMB);
+ admin.namespaces().setRetention(namespace, retention);
+ fail("Validation should have failed for " + retention);
+ } catch (PulsarAdminException e) {
+ assertTrue(e.getCause() instanceof BadRequestException);
+ assertTrue(e.getMessage().startsWith("Invalid retention policy"));
+ }
+ }
+
+ @Test
+ public void testRetentionPolicyValidationAsPartOfAllPolicies() throws Exception {
+ Policies policies = new Policies();
+ policies.replication_clusters = Sets.newHashSet(testLocalCluster);
+
+ assertValidRetentionPolicyAsPartOfAllPolicies(policies, 0, 0);
+ assertValidRetentionPolicyAsPartOfAllPolicies(policies, -1, -1);
+ assertValidRetentionPolicyAsPartOfAllPolicies(policies, 1, 1);
+
+ // should not pass validation
+ assertInvalidRetentionPolicyAsPartOfAllPolicies(policies, 1, 0);
+ assertInvalidRetentionPolicyAsPartOfAllPolicies(policies, 0, 1);
+ assertInvalidRetentionPolicyAsPartOfAllPolicies(policies, -1, 0);
+ assertInvalidRetentionPolicyAsPartOfAllPolicies(policies, 0, -1);
+ assertInvalidRetentionPolicyAsPartOfAllPolicies(policies, -2, 1);
+ assertInvalidRetentionPolicyAsPartOfAllPolicies(policies, 1, -2);
+ }
+
+ private void assertValidRetentionPolicyAsPartOfAllPolicies(Policies policies, int retentionTimeInMinutes,
+ int retentionSizeInMB) throws PulsarAdminException {
+ String namespace = this.testTenant + "/namespace-" + System.nanoTime();
+ RetentionPolicies retention = new RetentionPolicies(retentionTimeInMinutes, retentionSizeInMB);
+ policies.retention_policies = retention;
+ admin.namespaces().createNamespace(namespace, policies);
+ admin.namespaces().deleteNamespace(namespace);
+ }
+
+ private void assertInvalidRetentionPolicyAsPartOfAllPolicies(Policies policies, int retentionTimeInMinutes,
+ int retentionSizeInMB) {
+ String namespace = this.testTenant + "/namespace-" + System.nanoTime();
+ try {
+ RetentionPolicies retention = new RetentionPolicies(retentionTimeInMinutes, retentionSizeInMB);
+ policies.retention_policies = retention;
+ admin.namespaces().createNamespace(namespace, policies);
+ fail("Validation should have failed for " + retention);
+ } catch (PulsarAdminException e) {
+ assertTrue(e.getCause() instanceof BadRequestException);
+ assertTrue(e.getMessage().startsWith("Invalid retention policy"));
+ }
+ }
+}
diff --git a/pulsar-broker/src/test/java/org/apache/pulsar/broker/service/PersistentTopicE2ETest.java b/pulsar-broker/src/test/java/org/apache/pulsar/broker/service/PersistentTopicE2ETest.java
index 5dc3d2e..e20c18d 100644
--- a/pulsar-broker/src/test/java/org/apache/pulsar/broker/service/PersistentTopicE2ETest.java
+++ b/pulsar-broker/src/test/java/org/apache/pulsar/broker/service/PersistentTopicE2ETest.java
@@ -744,7 +744,7 @@ public class PersistentTopicE2ETest extends BrokerTestBase {
assertNotNull(pulsar.getBrokerService().getTopicReference(topicName));
// Remove retention
- admin.namespaces().setRetention("prop/ns-abc", new RetentionPolicies(0, 10));
+ admin.namespaces().setRetention("prop/ns-abc", new RetentionPolicies());
Thread.sleep(300);
// 2. Topic is not GCed with live connection
@@ -787,7 +787,7 @@ public class PersistentTopicE2ETest extends BrokerTestBase {
assertNotNull(pulsar.getBrokerService().getTopicReference(topicName));
// Remove retention
- admin.namespaces().setRetention("prop/ns-abc", new RetentionPolicies(0, 10));
+ admin.namespaces().setRetention("prop/ns-abc", new RetentionPolicies());
Thread.sleep(300);
// 2. Topic is not GCed with live connection
@@ -834,7 +834,7 @@ public class PersistentTopicE2ETest extends BrokerTestBase {
assertTrue(pulsar.getBrokerService().getTopicReference(topicName).isPresent());
// Remove retention
- admin.namespaces().setRetention("prop/ns-abc", new RetentionPolicies(0, 10));
+ admin.namespaces().setRetention("prop/ns-abc", new RetentionPolicies());
Thread.sleep(300);
// 2. Topic is not GCed with live connection
@@ -1684,4 +1684,4 @@ public class PersistentTopicE2ETest extends BrokerTestBase {
assertEquals(msg.getValue(), "test");
assertEquals(msg.getEventTime(), 5);
}
-}
\ No newline at end of file
+}