From 4ce3296a8f2890fd0b3ecfc9cc60275db1009fed Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Sat, 7 Dec 2024 00:46:24 +0200 Subject: [PATCH] [fix][cli] Fix set-retention with >2GB size value for topic policy --- .../pulsar/admin/cli/CmdTopicPolicies.java | 9 ++-- .../apache/pulsar/admin/cli/CmdTopics.java | 8 ++-- .../admin/cli/CmdTopicPoliciesTest.java | 47 +++++++++++++++++++ .../pulsar/admin/cli/TestCmdNamespaces.java | 5 +- .../pulsar/admin/cli/TestCmdTopics.java | 7 +++ 5 files changed, 65 insertions(+), 11 deletions(-) create mode 100644 pulsar-client-tools/src/test/java/org/apache/pulsar/admin/cli/CmdTopicPoliciesTest.java diff --git a/pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdTopicPolicies.java b/pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdTopicPolicies.java index 10850d107edf5..9a5714cb58ce0 100644 --- a/pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdTopicPolicies.java +++ b/pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdTopicPolicies.java @@ -29,7 +29,6 @@ import java.util.function.Supplier; import java.util.stream.Collectors; import org.apache.commons.lang3.StringUtils; -import org.apache.pulsar.cli.converters.picocli.ByteUnitToIntegerConverter; import org.apache.pulsar.cli.converters.picocli.ByteUnitToLongConverter; import org.apache.pulsar.cli.converters.picocli.TimeUnitToMillisConverter; import org.apache.pulsar.cli.converters.picocli.TimeUnitToSecondsConverter; @@ -546,8 +545,8 @@ private class SetRetention extends CliCommand { + "For example, 4096, 10M, 16G, 3T. The size unit suffix character can be k/K, m/M, g/G, or t/T. " + "If the size unit suffix is not specified, the default unit is bytes. " + "0 or less than 1MB means no retention and -1 means infinite size retention", required = true, - converter = ByteUnitToIntegerConverter.class) - private Integer sizeLimit; + converter = ByteUnitToLongConverter.class) + private Long sizeLimit; @Option(names = { "--global", "-g" }, description = "Whether to set this policy globally. " + "If set to true, the policy is replicated to other clusters asynchronously, " @@ -560,8 +559,8 @@ void run() throws PulsarAdminException { final int retentionTimeInMin = retentionTimeInSec != -1 ? (int) TimeUnit.SECONDS.toMinutes(retentionTimeInSec) : retentionTimeInSec.intValue(); - final int retentionSizeInMB = sizeLimit != -1 - ? (int) (sizeLimit / (1024 * 1024)) + final long retentionSizeInMB = sizeLimit != -1 + ? (sizeLimit / (1024 * 1024)) : sizeLimit; getTopicPolicies(isGlobal).setRetention(persistentTopic, new RetentionPolicies(retentionTimeInMin, retentionSizeInMB)); diff --git a/pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdTopics.java b/pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdTopics.java index 8dd6b664462ea..22073b1a89dc9 100644 --- a/pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdTopics.java +++ b/pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdTopics.java @@ -1858,8 +1858,8 @@ private class SetRetention extends CliCommand { + "For example, 4096, 10M, 16G, 3T. The size unit suffix character can be k/K, m/M, g/G, or t/T. " + "If the size unit suffix is not specified, the default unit is bytes. " + "0 or less than 1MB means no retention and -1 means infinite size retention", required = true, - converter = ByteUnitToIntegerConverter.class) - private Integer sizeLimit; + converter = ByteUnitToLongConverter.class) + private Long sizeLimit; @Override void run() throws PulsarAdminException { @@ -1867,8 +1867,8 @@ void run() throws PulsarAdminException { final int retentionTimeInMin = retentionTimeInSec != -1 ? (int) TimeUnit.SECONDS.toMinutes(retentionTimeInSec) : retentionTimeInSec.intValue(); - final int retentionSizeInMB = sizeLimit != -1 - ? (int) (sizeLimit / (1024 * 1024)) + final long retentionSizeInMB = sizeLimit != -1 + ? (sizeLimit / (1024 * 1024)) : sizeLimit; getTopics().setRetention(persistentTopic, new RetentionPolicies(retentionTimeInMin, retentionSizeInMB)); } diff --git a/pulsar-client-tools/src/test/java/org/apache/pulsar/admin/cli/CmdTopicPoliciesTest.java b/pulsar-client-tools/src/test/java/org/apache/pulsar/admin/cli/CmdTopicPoliciesTest.java new file mode 100644 index 0000000000000..59d3be3901935 --- /dev/null +++ b/pulsar-client-tools/src/test/java/org/apache/pulsar/admin/cli/CmdTopicPoliciesTest.java @@ -0,0 +1,47 @@ +/* + * 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.admin.cli; + +import static org.mockito.ArgumentMatchers.anyBoolean; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; +import org.apache.pulsar.client.admin.PulsarAdmin; +import org.apache.pulsar.client.admin.TopicPolicies; +import org.apache.pulsar.common.policies.data.RetentionPolicies; +import org.testng.annotations.Test; + +public class CmdTopicPoliciesTest { + + @Test + public void testSetRetentionCmd() throws Exception { + TopicPolicies topicPolicies = mock(TopicPolicies.class); + + PulsarAdmin admin = mock(PulsarAdmin.class); + when(admin.topicPolicies(anyBoolean())).thenReturn(topicPolicies); + + CmdTopicPolicies cmd = new CmdTopicPolicies(() -> admin); + + cmd.run("set-retention public/default/topic -s 2T -t 200d".split("\\s+")); + + verify(topicPolicies, times(1)).setRetention("persistent://public/default/topic", + new RetentionPolicies(200 * 24 * 60, 2 * 1024 * 1024)); + } +} \ No newline at end of file diff --git a/pulsar-client-tools/src/test/java/org/apache/pulsar/admin/cli/TestCmdNamespaces.java b/pulsar-client-tools/src/test/java/org/apache/pulsar/admin/cli/TestCmdNamespaces.java index f9ce84411c6c0..4ed0880d29edc 100644 --- a/pulsar-client-tools/src/test/java/org/apache/pulsar/admin/cli/TestCmdNamespaces.java +++ b/pulsar-client-tools/src/test/java/org/apache/pulsar/admin/cli/TestCmdNamespaces.java @@ -46,7 +46,8 @@ public void testSetRetentionCmd() throws Exception { CmdNamespaces cmd = new CmdNamespaces(() -> admin); - cmd.run("set-retention public/default -s 2T -t 2h".split("\\s+")); - verify(namespaces, times(1)).setRetention("public/default", new RetentionPolicies(120, 2 * 1024 * 1024)); + cmd.run("set-retention public/default -s 2T -t 200d".split("\\s+")); + verify(namespaces, times(1)).setRetention("public/default", + new RetentionPolicies(200 * 24 * 60, 2 * 1024 * 1024)); } } diff --git a/pulsar-client-tools/src/test/java/org/apache/pulsar/admin/cli/TestCmdTopics.java b/pulsar-client-tools/src/test/java/org/apache/pulsar/admin/cli/TestCmdTopics.java index fc98b14392c3e..bd926edc5a808 100644 --- a/pulsar-client-tools/src/test/java/org/apache/pulsar/admin/cli/TestCmdTopics.java +++ b/pulsar-client-tools/src/test/java/org/apache/pulsar/admin/cli/TestCmdTopics.java @@ -50,6 +50,7 @@ import org.apache.pulsar.client.impl.MessageIdImpl; import org.apache.pulsar.common.naming.TopicDomain; import org.apache.pulsar.common.policies.data.ManagedLedgerInternalStats.LedgerInfo; +import org.apache.pulsar.common.policies.data.RetentionPolicies; import org.mockito.Mockito; import org.testng.Assert; import org.testng.annotations.AfterMethod; @@ -260,4 +261,10 @@ public void testRunDeleteTopicsFromFileWithException() throws PulsarAdminExcepti mockTopics = mock(Topics.class); } + @Test + public void testSetRetentionCmd() throws Exception { + cmdTopics.run("set-retention public/default/topic -s 2T -t 200d".split("\\s+")); + verify(mockTopics, times(1)).setRetention("persistent://public/default/topic", + new RetentionPolicies(200 * 24 * 60, 2 * 1024 * 1024)); + } }