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

[BUGFIX] cannot cleanup expired data after managed-ledger restart #10087

Conversation

wuzhanpeng
Copy link
Contributor

This pull request resolves #10086

Motivation

Although #7111 and #9136 solved the problems of

  1. current ledger is full and cannot be rolled
  2. cursor has subscribed to an expired ledger, which makes the cleaning thread unable to clean up

respectively. However, when we close and open a managed-ledger(such like brokers shutdown unexpectedly or topics unload due to rebalance), the managed-ledger will create an empty ledger after initialization causing the current cleanup logic failed to take effect. Therefore I think we need a more unified entrance to solve the cursor update problem, and then further solve the problem of clearing expired ledgers.

Expected Behavior

  1. Able to cleanup expired data after managed-ledger re-open
  2. A more unified entrance to implement cursor update

Modification

  1. move the cursor update logic to internalTrimConsumedLedgers from ManagedLedgerImpl's callback createComplete
  2. update lastConfirmedEntry if necessary when updating cursors

try {
log.info("Reset cursor:{} to {} since ledger consumed completely", cursor, lastAckedPosition);
if (lastConfirmedEntry.compareTo(lastAckedPosition) < 0) {
lastConfirmedEntry = lastAckedPosition;
Copy link
Contributor

Choose a reason for hiding this comment

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

This will break the behavior of the lastConfirmedEntry because we will get such as (10, -1) when get the last confirm entry.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I have made a reminder in the place of Modification above. If we don’t modify lastConfirmedEntry here, the updateCursor can’t really succeed and exit with message [{}] Failed mark delete due to invalid markDelete {} is ahead of last-confirmed-entry {} for cursor [{}]. Then you will find that the state of cursor in memory is inconsistent with the metadata. This scenario may become difficult to understand.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's better to fix the cursor update behavior, not the lastConfirmedEntry. We can treat the (x, -1) as a specific mark delete position for the cursor.

@codelipenghui
Copy link
Contributor

@merlimat @eolivelli Please help review this PR thanks

@codelipenghui codelipenghui merged commit b6ca040 into apache:master May 23, 2021
yangl pushed a commit to yangl/pulsar that referenced this pull request Jun 23, 2021
…ache#10087)

This pull request resolves apache#10086 

## Motivation

Although apache#7111 and apache#9136 solved the problems of 
1. current ledger is full and cannot be rolled
2. cursor has subscribed to an expired ledger, which makes the cleaning thread unable to clean up

respectively. However, when we close and open a managed-ledger(such like brokers shutdown unexpectedly or topics unload due to rebalance), the managed-ledger will create an empty ledger after initialization causing the current cleanup logic failed to take effect. Therefore I think we need a more unified entrance to solve the cursor update problem, and then further solve the problem of clearing expired ledgers.

## Expected Behavior

1. Able to cleanup expired data after managed-ledger re-open
2. A more unified entrance to implement cursor update

## Modification

1. move the cursor update logic to `internalTrimConsumedLedgers` from `ManagedLedgerImpl`'s callback `createComplete`
2. update `lastConfirmedEntry` if necessary when updating cursors
bharanic-dev pushed a commit to bharanic-dev/pulsar that referenced this pull request Mar 18, 2022
…ache#10087)

This pull request resolves apache#10086 

## Motivation

Although apache#7111 and apache#9136 solved the problems of 
1. current ledger is full and cannot be rolled
2. cursor has subscribed to an expired ledger, which makes the cleaning thread unable to clean up

respectively. However, when we close and open a managed-ledger(such like brokers shutdown unexpectedly or topics unload due to rebalance), the managed-ledger will create an empty ledger after initialization causing the current cleanup logic failed to take effect. Therefore I think we need a more unified entrance to solve the cursor update problem, and then further solve the problem of clearing expired ledgers.

## Expected Behavior

1. Able to cleanup expired data after managed-ledger re-open
2. A more unified entrance to implement cursor update

## Modification

1. move the cursor update logic to `internalTrimConsumedLedgers` from `ManagedLedgerImpl`'s callback `createComplete`
2. update `lastConfirmedEntry` if necessary when updating cursors
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUGFIX] cannot cleanup expired data after managed-ledger restart
4 participants