Re: [PR] [improve][broker] PIP-315: Configurable max delay limit for delayed delivery [pulsar]

["lhotari (via GitHub)" <gi...@apache.org>]

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: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org