[broker] Add config to allow deliverAt time to be strictly honored #16068
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
michaeljmarshall
added
doc
michaeljmarshall
requested review from
eolivelli,
gaoran10,
lhotari,
merlimat and
urfreespace
lhotari
approved these changes
Jun 15, 2022
Member
lhotari
left a comment
lhotari
left a comment
There was a problem hiding this comment.
LGTM. Good work @michaeljmarshall
eolivelli
approved these changes
Jun 15, 2022
pulsar-broker/src/test/java/org/apache/pulsar/broker/delayed/InMemoryDeliveryTrackerTest.java
Outdated
Show resolved
Hide resolved
nicoloboschi
approved these changes
Jun 15, 2022
codelipenghui
approved these changes
Jun 15, 2022
Contributor
codelipenghui
left a comment
codelipenghui
left a comment
There was a problem hiding this comment.
LGTM, just left a comment about the configuration description.
nicoloboschi
pushed a commit
to datastax/pulsar
that referenced
this pull request
Jun 15, 2022
merlimat
pushed a commit
that referenced
this pull request
Jul 15, 2022
…16068) * [broker] Add config to allow deliverAt time to be strictly honored * Fix checkstyle error (this is what happens why you change names last minute) * Improve documentation; add private final modifiers 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. * 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. New tests are added as part of this change. This is a new feature that maintains backwards compatibility.
merlimat
pushed a commit
that referenced
this pull request
Jul 15, 2022
…16068) * [broker] Add config to allow deliverAt time to be strictly honored * Fix checkstyle error (this is what happens why you change names last minute) * Improve documentation; add private final modifiers 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. * 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. New tests are added as part of this change. This is a new feature that maintains backwards compatibility.
mattisonchao
added
cherry-picked/branch-2.9
momo-jun
added a commit
to momo-jun/pulsar
that referenced
this pull request
Dec 9, 2022
4 tasks
momo-jun
added a commit
to momo-jun/pulsar
that referenced
this pull request
Dec 9, 2022
This reverts commit fd4684a.
momo-jun
added a commit
to momo-jun/pulsar
that referenced
this pull request
Dec 9, 2022
4 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
The current implementation for
InMemoryDelayedDeliveryTrackerallows messages to deliver early when theirdeliverAttime is withintickTimeMillisfrom now. This is an optimization that ensures messages deliver around thedeliverAttime. However, some use cases require that messages do not deliver before thedeliverAttime. (Note that the client api includes adeliverAftermethod 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 thedeliverAttime is greater than or equal tonow. Note that a tradeoff here is that messages will be later than thedeliverAttime.There are two factors that will determine how late messages will get to consumers. The first is the topic's
DelayedDeliveryTickTimeMillisand the second is the broker'sdelayedDeliveryTickTimeMillis. 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 theHashedWheelTimer, and as a result, can compound with the topic's delay to make a message deliver even later.Modifications
isDelayedDeliveryDeliverAtTimeStrict. This config defaults tofalseto maintain the original behavior.InMemoryDelayedDeliveryTracker#addMessagemethod so that it will return false whendeliverAt <= getCutoffTime()instead of justdeliverAt <= getCutoffTime().InMemoryDelayedDeliveryTracker#getCutoffTimemethod that returns the right cutoff time based on the value ofisDelayedDeliveryDeliverAtTimeStrict. This is the core logical change.InMemoryDelayedDeliveryTracker#updateTimerso that it will not schedule a tick to run sooner that the most recent tick run plus thetickTimeMillis. 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 thetickTimeMillis.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.
doc