KevinLiLu commented on code in PR #21798:
URL: https://github.com/apache/pulsar/pull/21798#discussion_r1457012662
##########
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:
@lhotari I like this suggestion a lot although it might be better to pick it
up as a separate PIP. I can see many other configurations benefiting from this
change so it might be worth it to add support for all configurations at once.
Just throwing out some thoughts here:
1. We could maintain backwards compatibility by adding a new set of
configurations (one per existing timed config) which excludes the time portion
from the config key. Example new config: `delayedDeliveryMaxDelay=PT1H15M30S`.
We would have to handle the case in which the user has provided both formats
(either throw exception or just pick one).
2. Some people were concerned about the overhead cost of converting time
(ex. my original proposal used seconds instead of millis for readability) so we
may want to have the underlying implementation store the millis value to avoid
making extra calculations. I see that `Duration` only stores seconds/nanos so
calling `Duration#toMillis()` would involve an extra calculation to convert
second/nanos to millis.
--
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]
