Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[improve][broker]Improve PersistentMessageExpiryMonitor expire speed when ledger not existed #17842

Merged
merged 4 commits into from
Oct 3, 2022

Conversation

AnonHxy
Copy link
Contributor

@AnonHxy AnonHxy commented Sep 26, 2022

Motivation

Speed up the process of PersistentMessageExpiryMonitor expiring message when ledgers not exist.

For details:

Without consumer acking messages, the PersistentMessageExpiryMonitor expires messages process could be very slow if the ledger PersistentMessageExpiryMonitor be read has been deleted from the bookkeeper.

The root cause is that, when autoSkipNonRecoverableData is true and the ledger to be read has been deleted, the expireMessages will read entry failed and set the markDeletePosition as the failed position, see Line194. And then waiting to be scheduled(defalut value is 5min)

public void findEntryFailed(ManagedLedgerException exception, Optional<Position> failedReadPosition, Object ctx) {
if (log.isDebugEnabled()) {
log.debug("[{}][{}] Finding expired entry operation failed", topicName, subName, exception);
}
if (autoSkipNonRecoverableData && failedReadPosition.isPresent()
&& (exception instanceof NonRecoverableLedgerException)) {
log.warn("[{}][{}] read failed from ledger at position:{} : {}", topicName, subName, failedReadPosition,
exception.getMessage());
findEntryComplete(failedReadPosition.get(), ctx);
}
expirationCheckInProgress = FALSE;

In the next scheduled, the PersistentMessageExpiryMonitor will read the next position of markDeletePosition , which is also not existed because they may belong to the same ledger.

So the markDeletePosition just go forward 1 every 5min. And if there are many deleted ledgers, it will lead to the ttl expire lose efficacy

Modifications

If read entry failed with ledger not existed exceptions, we will set the markDeletePosition as the last position of the deleted ledger.

Verifying this change

  • Make sure that the change passes the CI checks.
  • org.apache.pulsar.broker.service.PersistentMessageFinderTest#testMessageExpiryWithTimestampNonRecoverableException has coverd this change

Does this pull request potentially affect one of the following parts:

Documentation

  • doc-not-needed
    (Please explain why)

Matching PR in forked repository

PR in forked repository: AnonHxy#4

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Sep 26, 2022
@AnonHxy AnonHxy self-assigned this Sep 26, 2022
@AnonHxy AnonHxy changed the title [improve][broker]Improve PersistentMessageExpiryMonitor expire speed if meet NonRecove… [improve][broker]Improve PersistentMessageExpiryMonitor expire speed when ledger not existed Sep 26, 2022
@AnonHxy AnonHxy force-pushed the opt_expire_markdelete branch from 83b4680 to 36ee4d7 Compare September 27, 2022 08:38
Copy link
Contributor

@lordcheng10 lordcheng10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@AnonHxy AnonHxy force-pushed the opt_expire_markdelete branch from 3b98253 to f074db0 Compare September 29, 2022 06:31
@AnonHxy
Copy link
Contributor Author

AnonHxy commented Sep 30, 2022

@codelipenghui @Technoboy- PTAL

@AnonHxy
Copy link
Contributor Author

AnonHxy commented Oct 3, 2022

/pulsarbot run-failure-checks

@AnonHxy AnonHxy merged commit af11c32 into apache:master Oct 3, 2022
@AnonHxy AnonHxy added the type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages label Oct 3, 2022
@AnonHxy AnonHxy added this to the 2.12.0 milestone Oct 3, 2022
congbobo184 pushed a commit that referenced this pull request Nov 14, 2022
…when ledger not existed (#17842)

(cherry picked from commit af11c32)
@congbobo184 congbobo184 added the cherry-picked/branch-2.9 Archived: 2.9 is end of life label Nov 14, 2022
congbobo184 pushed a commit that referenced this pull request Nov 27, 2022
…when ledger not existed (#17842)

(cherry picked from commit af11c32)
liangyepianzhou pushed a commit that referenced this pull request Dec 13, 2022
…when ledger not existed (#17842)

(cherry picked from commit af11c32)
nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Jan 10, 2023
…when ledger not existed (apache#17842)

(cherry picked from commit af11c32)
(cherry picked from commit fcc98d1)
nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Jan 11, 2023
…when ledger not existed (apache#17842)

(cherry picked from commit af11c32)
(cherry picked from commit fcc98d1)
Technoboy- pushed a commit that referenced this pull request Feb 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-picked/branch-2.9 Archived: 2.9 is end of life cherry-picked/branch-2.10 cherry-picked/branch-2.11 doc-not-needed Your PR changes do not impact docs release/2.9.4 release/2.10.3 release/2.11.1 type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants