lhotari commented on code in PR #21798:
URL: https://github.com/apache/pulsar/pull/21798#discussion_r1449280741
##########
pulsar-client-admin-api/src/main/java/org/apache/pulsar/common/policies/data/DelayedDeliveryPolicies.java:
##########
@@ -26,10 +26,12 @@
public interface DelayedDeliveryPolicies {
long getTickTime();
boolean isActive();
+ long getMaxDeliveryDelayInMillis();
interface Builder {
Builder tickTime(long tickTime);
Builder active(boolean active);
+ Builder maxDeliveryDelayInMillis(long maxDeliveryDelayInMillis);
Review Comment:
I should have made this comment already in the PIP phase, but I'll bring it
up anyways. :)
Have you considered using `Duration` as the type here? I know that we have
multiple ways of dealing with durations and long values are commonly used in
Pulsar APIs, but perhaps we could improve on this. There's a benefit in using
Duration in public APIs since it makes it clear what the unit is and there's
less chances for mistakes. I think that internally it would be fine to store it
in a long.
btw. It would be nice to have support for parsing durations in configuration
files so that there wouldn't be a need to calculate things in milliseconds when
configuring `delayedDeliveryMaxDelayInMillis` in broker.conf, but that would
first require adding support for configuring Duration in configuration files.
It would be great if [ISO-8601 Duration format would be
supported](https://en.wikipedia.org/wiki/ISO_8601#Durations). For example
"PT1H15M30S" would mean 1 hour 15 minutes and 30 seconds. "PT0.001S" would mean
1 millisecond. It should be fairly simple to add support for creating
java.time.Duration instances to the configuration framework by adding converter
methods to org.apache.pulsar.common.util.FieldParser class in pulsar-common
module. java.time.Duration#parse supports parsing ISO-8601 duration format.
The switch to use java.time.Duration as the primary type for durations in
Pulsar would be something that I can bring up to discussion on the dev mailing
list. This is something that I have planned to do earlier and now I noticed
that it would be great to finally move towards this direction.
I'm also fine in the current way if there's a need to get this completed
asap as-is.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
