You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2022/06/15 05:28:19 UTC
[GitHub] [pulsar] michaeljmarshall opened a new pull request, #16068: [broker] Add config to allow deliverAt time to be strictly honored
michaeljmarshall opened a new pull request, #16068:
URL: https://github.com/apache/pulsar/pull/16068
### Motivation
The current implementation for `InMemoryDelayedDeliveryTracker` allows messages to deliver early when their `deliverAt` time is within `tickTimeMillis` from now. This is an optimization that ensures messages deliver around the `deliverAt` time. However, some use cases require that messages do not deliver before the `deliverAt` time. (Note that the client api includes a `deliverAfter` method that implies messages won't deliver before some duration of time.)
In order to support this alternative implementation, this PR adds a broker configuration named `isDelayedDeliveryDeliverAtTimeStrict`. When true, messages will only deliver when the `deliverAt` time is greater than or equal to `now`. Note that a tradeoff here is that messages will be later than the `deliverAt` time.
There are two factors that will determine how late messages will get to consumers. The first is the topic's `DelayedDeliveryTickTimeMillis` and the second is the broker's `delayedDeliveryTickTimeMillis`. The first will determine how frequently a timer will be scheduled to deliver delayed messages. The second is used to determine the tick time of the `HashedWheelTimer`, and as a result, can compound with the topic's delay to make a message deliver even later.
### Modifications
* Add broker config named `isDelayedDeliveryDeliverAtTimeStrict`. This config defaults to `false` to maintain the original behavior.
* Update the `InMemoryDelayedDeliveryTracker#addMessage` method so that it will return false when `deliverAt <= getCutoffTime()` instead of just `deliverAt <= getCutoffTime()`.
* Update documentation in several places.
* Implement `InMemoryDelayedDeliveryTracker#getCutoffTime` method that returns the right cutoff time based on the value of `isDelayedDeliveryDeliverAtTimeStrict`. This is the core logical change.
* Update `InMemoryDelayedDeliveryTracker#updateTimer` so that it will not schedule a tick to run sooner that the most recent tick run plus the `tickTimeMillis`. This will ensure the timer is not run too frequently. It is also backwards compatible since the existing feature will deliver any messages that were within now plus the `tickTimeMillis`.
* Add new tests to cover the new configuration.
### Verifying this change
New tests are added as part of this change.
### Does this pull request potentially affect one of the following parts:
This is a new feature that maintains backwards compatibility.
--
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
[GitHub] [pulsar] codelipenghui commented on a diff in pull request #16068: [broker] Add config to allow deliverAt time to be strictly honored
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on code in PR #16068:
URL: https://github.com/apache/pulsar/pull/16068#discussion_r897813964
##########
conf/broker.conf:
##########
@@ -514,9 +514,18 @@ delayedDeliveryEnabled=true
# Control the tick time for when retrying on delayed delivery,
# affecting the accuracy of the delivery time compared to the scheduled time.
+# Note that this time is used to configure the HashedWheelTimer's tick time for the
+# InMemoryDelayedDeliveryTrackerFactory (the default DelayedDeliverTrackerFactory).
# Default is 1 second.
delayedDeliveryTickTimeMillis=1000
+# When using the InMemoryDelayedDeliveryTrackerFactory (the default DelayedDeliverTrackerFactory), whether
+# the deliverAt time is strictly followed. When false, messages may be sent to consumers before the
+# deliverAt time by as much as the tickTimeMillis. When true, messages will not be sent to consumer until
+# the deliverAt time has passed, and they may be as late as the deliverAt time plus the tickTimeMillis for the
+# topic plus the delayedDeliveryTickTimeMillis.
+isDelayedDeliveryDeliverAtTimeStrict=false
Review Comment:
It's better to add some information about set to false which allows sending to consumers in advance for tick time but can reduce the overhead of just maintaining the delayed index in a very short time period. Otherwise, users can't find any reason to set this to false and defaults to false too.
--
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
[GitHub] [pulsar] michaeljmarshall commented on a diff in pull request #16068: [broker] Add config to allow deliverAt time to be strictly honored
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on code in PR #16068:
URL: https://github.com/apache/pulsar/pull/16068#discussion_r898123636
##########
conf/broker.conf:
##########
@@ -514,9 +514,18 @@ delayedDeliveryEnabled=true
# Control the tick time for when retrying on delayed delivery,
# affecting the accuracy of the delivery time compared to the scheduled time.
+# Note that this time is used to configure the HashedWheelTimer's tick time for the
+# InMemoryDelayedDeliveryTrackerFactory (the default DelayedDeliverTrackerFactory).
# Default is 1 second.
delayedDeliveryTickTimeMillis=1000
+# When using the InMemoryDelayedDeliveryTrackerFactory (the default DelayedDeliverTrackerFactory), whether
+# the deliverAt time is strictly followed. When false, messages may be sent to consumers before the
+# deliverAt time by as much as the tickTimeMillis. When true, messages will not be sent to consumer until
+# the deliverAt time has passed, and they may be as late as the deliverAt time plus the tickTimeMillis for the
+# topic plus the delayedDeliveryTickTimeMillis.
+isDelayedDeliveryDeliverAtTimeStrict=false
Review Comment:
Done.
--
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
[GitHub] [pulsar] eolivelli commented on a diff in pull request #16068: [broker] Add config to allow deliverAt time to be strictly honored
Posted by GitBox <gi...@apache.org>.
eolivelli commented on code in PR #16068:
URL: https://github.com/apache/pulsar/pull/16068#discussion_r897600801
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/delayed/InMemoryDeliveryTrackerTest.java:
##########
@@ -40,27 +43,38 @@
import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicLong;
+import io.netty.util.concurrent.DefaultThreadFactory;
import lombok.Cleanup;
import org.apache.bookkeeper.mledger.impl.PositionImpl;
import org.apache.pulsar.broker.service.persistent.PersistentDispatcherMultipleConsumers;
+import org.awaitility.Awaitility;
+import org.testng.annotations.AfterClass;
import org.testng.annotations.Test;
@Test(groups = "broker")
public class InMemoryDeliveryTrackerTest {
+ // Create a single shared timer for the test.
+ Timer timer = new HashedWheelTimer(new DefaultThreadFactory("pulsar-in-memory-delayed-delivery-test"),
Review Comment:
nit: private final
--
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
[GitHub] [pulsar] michaeljmarshall commented on a diff in pull request #16068: [broker] Add config to allow deliverAt time to be strictly honored
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on code in PR #16068:
URL: https://github.com/apache/pulsar/pull/16068#discussion_r898109525
##########
conf/broker.conf:
##########
@@ -514,9 +514,18 @@ delayedDeliveryEnabled=true
# Control the tick time for when retrying on delayed delivery,
# affecting the accuracy of the delivery time compared to the scheduled time.
+# Note that this time is used to configure the HashedWheelTimer's tick time for the
+# InMemoryDelayedDeliveryTrackerFactory (the default DelayedDeliverTrackerFactory).
# Default is 1 second.
delayedDeliveryTickTimeMillis=1000
+# When using the InMemoryDelayedDeliveryTrackerFactory (the default DelayedDeliverTrackerFactory), whether
+# the deliverAt time is strictly followed. When false, messages may be sent to consumers before the
+# deliverAt time by as much as the tickTimeMillis. When true, messages will not be sent to consumer until
+# the deliverAt time has passed, and they may be as late as the deliverAt time plus the tickTimeMillis for the
+# topic plus the delayedDeliveryTickTimeMillis.
+isDelayedDeliveryDeliverAtTimeStrict=false
Review Comment:
Great point. I'll update the documentation.
--
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
[GitHub] [pulsar] michaeljmarshall merged pull request #16068: [broker] Add config to allow deliverAt time to be strictly honored
Posted by GitBox <gi...@apache.org>.
michaeljmarshall merged PR #16068:
URL: https://github.com/apache/pulsar/pull/16068
--
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
[GitHub] [pulsar] michaeljmarshall commented on a diff in pull request #16068: [broker] Add config to allow deliverAt time to be strictly honored
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on code in PR #16068:
URL: https://github.com/apache/pulsar/pull/16068#discussion_r898123254
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/delayed/InMemoryDeliveryTrackerTest.java:
##########
@@ -40,27 +43,38 @@
import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicLong;
+import io.netty.util.concurrent.DefaultThreadFactory;
import lombok.Cleanup;
import org.apache.bookkeeper.mledger.impl.PositionImpl;
import org.apache.pulsar.broker.service.persistent.PersistentDispatcherMultipleConsumers;
+import org.awaitility.Awaitility;
+import org.testng.annotations.AfterClass;
import org.testng.annotations.Test;
@Test(groups = "broker")
public class InMemoryDeliveryTrackerTest {
+ // Create a single shared timer for the test.
+ Timer timer = new HashedWheelTimer(new DefaultThreadFactory("pulsar-in-memory-delayed-delivery-test"),
Review Comment:
Done
--
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