Issue 9082: Broker expires messages one at a time after topic unload #9083
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
|
Notes for the reviewers: |
sijie
requested changes
Dec 29, 2020
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/OpFindNewest.java
Outdated
Show resolved
Hide resolved
managed-ledger/src/test/java/org/apache/bookkeeper/mledger/impl/ManagedCursorTest.java
Outdated
Show resolved
Hide resolved
pulsar-broker/src/test/java/org/apache/pulsar/broker/service/MessageTTLTest.java
Show resolved
Hide resolved
pulsar-broker/src/test/java/org/apache/pulsar/broker/service/MessageTTLTest.java
Outdated
Show resolved
Hide resolved
pulsar-broker/src/test/java/org/apache/pulsar/broker/service/MessageTTLTest.java
Outdated
Show resolved
Hide resolved
pulsar-broker/src/test/java/org/apache/pulsar/broker/service/MessageTTLTest.java
Outdated
Show resolved
Hide resolved
pulsar-broker/src/test/java/org/apache/pulsar/broker/service/MessageTTLTest.java
Outdated
Show resolved
Hide resolved
eolivelli
commented
Dec 29, 2020
managed-ledger/src/test/java/org/apache/bookkeeper/mledger/impl/ManagedCursorTest.java
Outdated
Show resolved
Hide resolved
pulsar-broker/src/test/java/org/apache/pulsar/broker/service/MessageTTLTest.java
Outdated
Show resolved
Hide resolved
pulsar-broker/src/test/java/org/apache/pulsar/broker/service/MessageTTLTest.java
Outdated
Show resolved
Hide resolved
pulsar-broker/src/test/java/org/apache/pulsar/broker/service/MessageTTLTest.java
Show resolved
Hide resolved
pulsar-broker/src/test/java/org/apache/pulsar/broker/service/MessageTTLTest.java
Outdated
Show resolved
Hide resolved
|
@eolivelli left my comments. |
|
@sijie I have just left one of your comments to address, probably I still do not understand how can enhance the test with lastMessageId |
eolivelli
commented
Dec 30, 2020
| .subscribe(); | ||
|
|
||
| MessageId lastMessageIdOnConsumer = consumer.getLastMessageId(); | ||
| log.info("lastMessageID written {}, lastMessageIdForConsumer {}", lastMessageId, lastMessageIdOnConsumer); |
There was a problem hiding this comment.
these two values are the same, even if the messages are expired
There was a problem hiding this comment.
You are right. I think you can get stats-internal to get the internal stats to get the markDeletePosition. If you see the markDeletePosition is moved, it means TTL is taking effect.
There was a problem hiding this comment.
very good @sijie
I have updated the test
|
@sijie can you please take another look ? I answered to your comments |
|
/pulsarbot run-failure-checks |
sijie
approved these changes
Jan 7, 2021
codelipenghui
pushed a commit
that referenced
this pull request
Jan 28, 2021
…9083) Fixes #9082 In case of topic unload the ManagedLedger rolls a new ledger and this confuses the search for messages to be expired. This is how OpFindNewest (the operation involved in message expiry, PersistentMessageExpiryMonitor) works: - first check if first message is expired - test in the last message - perform a search... At step two we are jumping to a position that is not valid, and the search ends immediately, returning only the first message Ensure that we are jumping only to a valid position, this change allows the PersistentMessageExpiryMonitor to find the best range of messages to expire. This change added tests: - unit test that covers the change - one reproducer for the problem, that now is fixes - another test case for the expected end-to-end behaviour for a consumer that waits too much and messages expire It can be also verified trying to reproduce manually the reproducer at #9082, after applying this patch the issue is no more reproducible (cherry picked from commit 7313b34)
zymap
pushed a commit
that referenced
this pull request
Mar 2, 2021
…9083) Fixes #9082 In case of topic unload the ManagedLedger rolls a new ledger and this confuses the search for messages to be expired. This is how OpFindNewest (the operation involved in message expiry, PersistentMessageExpiryMonitor) works: - first check if first message is expired - test in the last message - perform a search... At step two we are jumping to a position that is not valid, and the search ends immediately, returning only the first message Ensure that we are jumping only to a valid position, this change allows the PersistentMessageExpiryMonitor to find the best range of messages to expire. This change added tests: - unit test that covers the change - one reproducer for the problem, that now is fixes - another test case for the expected end-to-end behaviour for a consumer that waits too much and messages expire It can be also verified trying to reproduce manually the reproducer at #9082, after applying this patch the issue is no more reproducible (cherry picked from commit 7313b34)
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.
Fixes #9082
Motivation
In case of topic unload the ManagedLedger rolls a new ledger and this confuses the search for messages to be expired.
This is how OpFindNewest (the operation involved in message expiry, PersistentMessageExpiryMonitor) works:
At step two we are jumping to a position that is not valid, and the search ends immediately, returning only the first message
Modifications
Ensure that we are jumping only to a valid position, this change allows the PersistentMessageExpiryMonitor to find the best range of messages to expire.
Verifying this change
This change added tests:
It can be also verified trying to reproduce manually the reproducer at #9082, after applying this patch the issue is no more reproducible