[Messaging] Fix delay message block #10078
Merged
Conversation
This file contains 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
|
This PR based on the PR #10077. |
eolivelli
reviewed
Mar 30, 2021
| .subscriptionName("test") | ||
| .subscribe(); | ||
|
|
||
| message = dltConsumer.receive(10, TimeUnit.SECONDS); |
There was a problem hiding this comment.
is there a way to not rely on this timeout ?
I am afraid we are going to add a new flaky test
cc @lhotari
There was a problem hiding this comment.
Normally, we could receive messages from the dead letter topic.
315157973
reviewed
Mar 30, 2021
| .create(); | ||
|
|
||
| final int redeliverCnt = 10; | ||
| final int delayTimeSeconds = 10; |
There was a problem hiding this comment.
Now that the integration test has been taking a long time, can we reduce the time here?
Later receive message at first time will use more than 100 seconds
There was a problem hiding this comment.
I reduce the delay timeout to 5, thanks.
codelipenghui
added
release/2.7.2
type/bug
codelipenghui
requested review from
merlimat,
315157973,
hangc0276,
sijie and
zymap
|
/pulsarbot run-failure-checks |
1 similar comment
|
/pulsarbot run-failure-checks |
gaoran10
force-pushed
the
fix-delay-message-block
branch
from
0f72de0
to
9aeed9d
Compare
|
/pulsarbot run-failure-checks |
codelipenghui
approved these changes
Apr 9, 2021
zymap
pushed a commit
that referenced
this pull request
Apr 14, 2021
Currently, in the docker environment, if the consumer enables the retry feature and sets the retry topic in DeadLetterPolicy, the consumer will be blocked after receive retry messages many times. The delay TimerTask may run before reaching the timeout, we could find out that the last log `Timer triggered` run after the log `Start timer in 4958 millis` but we cloud compute the duration between these two logs is `4936`, it's less than `4958`, so the hasMessageAvailable check is false, if there are no more messages to read the delay messages reading will be blocked. Please check logs below. (cherry picked from commit 31f8315)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Based on the PR #10077
Motivation
Currently, in the docker environment, if the consumer enables the retry feature and sets the retry topic in DeadLetterPolicy, the consumer will be blocked after receive retry messages many times.
The delay TimerTask may run before reaching the timeout, we could find out that the last log
Timer triggeredrun after the logStart timer in 4958 millisbut we cloud compute the duration between these two logs is4936, it's less than4958, so the hasMessageAvailable check is false, if there are no more messages to read the delay messages reading will be blocked. Please check logs below.test environment
Macos 11.2.3 (20D91)
Docker image
apachepulsar/pulsar:2.7.1, run standalone mode use default configuration.filter related logs ▽ ▽ ▽
Modifications
change the hasMessageAvailable check for delay messages
Verifying this change
Add an integration test.
Does this pull request potentially affect one of the following parts:
If
yeswas chosen, please highlight the changes