-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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.
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
[improve][admin] Enable users to specify TTL options in units other than seconds #15657
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, and I left a minor suggestion
pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdTopicPolicies.java
Outdated
Show resolved
Hide resolved
PTAL |
@Parameter(names = { "--messageTTL", "-ttl" }, | ||
description = "Message TTL in seconds (or minutes, hours, days, weeks eg: 100m, 3h, 2d, 5w). " | ||
+ "When the value is set to `0`, TTL is disabled.", required = true) | ||
private String messageTTLStr; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will there be compatibility issues? Some scripts written by users cannot be used now because they do not contain units.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it's compatible. If the option does not contain a unit, its value is treated as a number of seconds.
pulsar/pulsar-common/src/main/java/org/apache/pulsar/common/util/RelativeTimeUtil.java
Lines 31 to 67 in 34673dd
public static long parseRelativeTimeInSeconds(String relativeTime) { | |
if (relativeTime.isEmpty()) { | |
throw new IllegalArgumentException("expiry time cannot be empty"); | |
} | |
int lastIndex = relativeTime.length() - 1; | |
char lastChar = relativeTime.charAt(lastIndex); | |
final char timeUnit; | |
if (!Character.isAlphabetic(lastChar)) { | |
// No unit specified, assume seconds | |
timeUnit = 's'; | |
lastIndex = relativeTime.length(); | |
} else { | |
timeUnit = Character.toLowerCase(lastChar); | |
} | |
long duration = Long.parseLong(relativeTime.substring(0, lastIndex)); | |
switch (timeUnit) { | |
case 's': | |
return duration; | |
case 'm': | |
return TimeUnit.MINUTES.toSeconds(duration); | |
case 'h': | |
return TimeUnit.HOURS.toSeconds(duration); | |
case 'd': | |
return TimeUnit.DAYS.toSeconds(duration); | |
case 'w': | |
return 7 * TimeUnit.DAYS.toSeconds(duration); | |
// No unit for months | |
case 'y': | |
return 365 * TimeUnit.DAYS.toSeconds(duration); | |
default: | |
throw new IllegalArgumentException("Invalid time unit '" + lastChar + "'"); | |
} | |
} |
Motivation
There were some options that weren't fixed in the previous PR, so I fixed them as well as the other options.
Verifying this change
Documentation
doc