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

[fix] Exchange data cleanup improvement #698

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

gaoran10
Copy link
Collaborator

@gaoran10 gaoran10 commented Oct 10, 2022

Motivation

The PR #619 changed the queue cursor to be durable, this will cause the exchange topic can't be cleaned up completely because one queue only handles part of the exchange data, and only these index positions could be acknowledged.

image

At checkpoint1, get the route mark delete position of exchange1 and exchange2, then get the last confirm position of the queue. The exchange1 route mark delete pos is 1:2, the exchange2 mark delete pos is 2:2, and the LAC of the queue may be equal with 3:2 or greater than 3:2 (because the routing message process is still running), when the mark delete pos of the queue reach the LAC at checkpoint1, it indicates that the index message 3:0, 3:1 and 3:2 were all acked, so we can ack the exchange1 with pos 1:2, and ack the exchange2 with pos 2:2.

Modifications

Add exchange data cleanup scheduled task for each queue.
Remove useless unAckMessages in AmqpConsumer.

Verifying this change

Add verification to make sure exchange data could be cleaned up completely.

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

If yes was chosen, please highlight the changes

  • Dependencies (does it add or upgrade a dependency): (no)
  • The public API: (no)
  • The schema: (no)
  • The default values of configurations: (no)
  • The wire protocol: (no)
  • The rest endpoints: (no)
  • The admin cli options: (no)
  • Anything that affects deployment: (no)

Documentation

Check the box below.

Need to update docs?

  • doc-required

    (If you need help on updating docs, create a doc issue)

  • no-need-doc

    (Please explain why)

  • doc

    (If this PR contains doc changes)

@gaoran10 gaoran10 requested review from codelipenghui and a team as code owners October 10, 2022 08:18
@github-actions github-actions bot added the no-need-doc This pr does not need any document label Oct 10, 2022
queueName, position.getLedgerId(), position.getEntryId()));
}
}
FutureUtil.waitForAll(futures).thenRun(this::scheduleExchangeClearTask);
Copy link
Contributor

Choose a reason for hiding this comment

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

When (router.getExchange().markDeleteAsync) throws an exception, the task will stop. Replace (thenRun) with (whenComplete)

Copy link
Collaborator Author

@gaoran10 gaoran10 Oct 14, 2022

Choose a reason for hiding this comment

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

Good catch!

Fixed, please take a look again, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-need-doc This pr does not need any document
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants