[fix] [broker] Expire messages according to ledger close time to avoid client clock skew #21940
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
[fix][test]Fix resource not close after method
...rc/main/java/org/apache/pulsar/broker/service/persistent/PersistentMessageExpiryMonitor.java
Outdated
Show resolved
Hide resolved
...rc/main/java/org/apache/pulsar/broker/service/persistent/PersistentMessageExpiryMonitor.java
Outdated
Show resolved
Hide resolved
...rc/main/java/org/apache/pulsar/broker/service/persistent/PersistentMessageExpiryMonitor.java
Outdated
Show resolved
Hide resolved
|
Regarding the PR title, I think we can change it to [fix] [broker] Expire messages according to ledger close time to avoid client clock skew |
codelipenghui
added
release/2.10.6
release/3.0.3
release/2.11.4
release/3.1.3
release/3.2.1
type/bug
315157973
changed the title
...rc/main/java/org/apache/pulsar/broker/service/persistent/PersistentMessageExpiryMonitor.java
Outdated
Show resolved
Hide resolved
pulsar-broker/src/test/java/org/apache/pulsar/broker/service/PersistentMessageFinderTest.java
Show resolved
Hide resolved
...rc/main/java/org/apache/pulsar/broker/service/persistent/PersistentMessageExpiryMonitor.java
Outdated
Show resolved
Hide resolved
pulsar-broker/src/test/java/org/apache/pulsar/broker/service/PersistentMessageFinderTest.java
Outdated
Show resolved
Hide resolved
codelipenghui
approved these changes
Jan 30, 2024
|
It looks like related to this change? I have re-triggered the test. |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #21940 +/- ##
============================================
+ Coverage 73.59% 73.63% +0.03%
- Complexity 32417 32486 +69
============================================
Files 1861 1863 +2
Lines 138678 138839 +161
Branches 15188 15222 +34
============================================
+ Hits 102060 102228 +168
+ Misses 28715 28710 -5
+ Partials 7903 7901 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
|
coderzc
reviewed
Feb 1, 2024
...rc/main/java/org/apache/pulsar/broker/service/persistent/PersistentMessageExpiryMonitor.java
Outdated
Show resolved
Hide resolved
|
@codelipenghui @coderzc PTAL |
coderzc
approved these changes
Feb 5, 2024
...rc/main/java/org/apache/pulsar/broker/service/persistent/PersistentMessageExpiryMonitor.java
Show resolved
Hide resolved
dao-jun
approved these changes
Feb 20, 2024
|
seems there is a test keeps failing, please check |
Technoboy-
pushed a commit
that referenced
this pull request
Feb 26, 2024
…d client clock skew (#21940)
|
@315157973 could you help to cherry-pick this PR to branch-3.0? I see conflicts. |
This was referenced Feb 29, 2024
gaoran10
pushed a commit
to gaoran10/pulsar
that referenced
this pull request
Mar 4, 2024
…d client clock skew (apache#21940) (cherry picked from commit 861618a)
mukesh-ctds
pushed a commit
to datastax/pulsar
that referenced
this pull request
Mar 4, 2024
…d client clock skew (apache#21940) (cherry picked from commit 861618a) (cherry picked from commit fee3a33)
lhotari
added
release/2.10.7
release/2.11.5
and removed
release/2.10.6
release/2.11.4
labels
mukesh-ctds
pushed a commit
to datastax/pulsar
that referenced
this pull request
Mar 6, 2024
…d client clock skew (apache#21940) (cherry picked from commit 861618a) (cherry picked from commit fee3a33)
heesung-sn
pushed a commit
to heesung-sn/pulsar
that referenced
this pull request
Mar 6, 2024
…d client clock skew (apache#21940)
heesung-sn
pushed a commit
to heesung-sn/pulsar
that referenced
this pull request
Mar 6, 2024
…d client clock skew (apache#21940) (cherry picked from commit 861618a)
heesung-sn
added a commit
that referenced
this pull request
Mar 7, 2024
…time to avoid client clock skew (#21940) (#22211) Co-authored-by: feynmanlin <[email protected]>
15 tasks
3 tasks
2 tasks
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.
Main Issue: #21347
Motivation
When the client machine's clock is incorrect (eg: set to 1 year later) and the Broker does not set the AppendBrokerTimestampMetadataInterceptor, the Ledger will not be cleaned up.
Because if the Broker's timestamp is not set, the expiration check will be based on the client's publish time.
As we discuss in https://lists.apache.org/thread/0vtjt436xvlg0fr4ogchkjdvjcp56mvz
Modifications
TTL not base the on wrong client publish time
Verifying this change
Documentation
doc-not-neededMatching PR in forked repository
PR in forked repository: 315157973#14