Skip to content

Introduce messages fence mechanism for Key_Shared subscription #6977

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.

Sign up for GitHub

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

Jump to bottom
Closed
codelipenghui wants to merge 5 commits into from
Closed

Introduce messages fence mechanism for Key_Shared subscription #6977

codelipenghui wants to merge 5 commits into from

Conversation

@codelipenghui
Copy link
Contributor

@codelipenghui codelipenghui commented May 18, 2020

Fixes #6554

Motivation

Introduce messages fence mechanism for Key_Shared subscription to guarantee ordering dispatching.

The fenced message means that the messages can't be delivered at this time. In Key_Shared subscription, the new consumer needs to wait for all messages before the first message that delivers to this consumer are acknowledged. The fencedMessagesContainer maintains all fenced messages by a map (mark-delete-position -> fenced messages). When the mark delete position of the cursor is greater than the mark-delete-position in the fencedMessagesContainer, it means the fenced messages will be lifted.

Modifications

Describe the modifications you've done.

Verifying this change

New unit tests added.

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

  • Does this pull request introduce a new feature? (no)
  • If yes, how is the feature documented? (docs)

@codelipenghui codelipenghui self-assigned this May 18, 2020
@codelipenghui codelipenghui added this to the 2.6.0 milestone May 18, 2020
@codelipenghui
Copy link
Contributor Author

codelipenghui commented May 18, 2020

@merlimat Please help take a look at this PR. This implementation is based on your idea #6554 (comment). Thanks for your great idea.

@codelipenghui codelipenghui mentioned this pull request May 19, 2020
Closed
Huanli-Meng
Huanli-Meng reviewed May 19, 2020
View reviewed changes
Copy link
Contributor

@Huanli-Meng Huanli-Meng left a comment

Choose a reason for hiding this comment

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

leave my comments, PTAL

@sijie sijie mentioned this pull request May 20, 2020
Closed
@merlimat
Copy link
Contributor

merlimat commented May 20, 2020

@codelipenghui This is not quite what I was describing in the comment. I don't think it's desirable to "fence" the individual messages, rather just keeping a pointer to where the consumer joined. I have already a fix (working in prod) but it's backed up on #6791.

@codelipenghui
Copy link
Contributor Author

codelipenghui commented May 21, 2020

@merlimat I have added a pointer to indicate where the consumer joined and if the pointer has been mark deleted, the dispatcher dispatch messages to the consumer. The fenced messages just messages buffer, If messages don't belong to this consumer, the messages can continue dispatch to other consumers. If the buffer is full, the dispatcher will stop dispatch messages. I'm not sure if this is a good approach, the main idea does not stop dispatch messages to other consumers.

I have already a fix (working in prod) but it's backed up on #6791.

Is this mean the fix depends on #6791? Or the fix is only for the consistent hash approach.

@codelipenghui codelipenghui force-pushed the key_shared_ordering branch from 787ef45 to 881bb80 Compare May 27, 2020 05:17
@codelipenghui
Copy link
Contributor Author

codelipenghui commented May 27, 2020

/pulsarbot run-failure-checks

@Huanli-Meng
Copy link
Contributor

Huanli-Meng commented May 27, 2020

should the key_shared subscription in messaging section be updated?

@feeblefakie
Copy link

feeblefakie commented May 27, 2020

@codelipenghui
Just a question.
So this and #6791 are solving the same issue with different approaches but the both will be applied ?
#6791 looks like more promising but not fully sure.
Could I get some information about it ?

@codelipenghui
Copy link
Contributor Author

codelipenghui commented May 27, 2020 •
edited
Loading

@feeblefakie #6791 introduces a consistent hash approach for the consumer selector. The current implementation is a hash range split approach.

This PR is to fix #6554

@codelipenghui codelipenghui mentioned this pull request May 30, 2020
Merged
@codelipenghui
Copy link
Contributor Author

codelipenghui commented Jun 5, 2020

close via #7106

@feeblefakie feeblefakie mentioned this pull request Jul 6, 2020
Closed
@sijie sijie mentioned this pull request Jul 6, 2020
Closed
@codelipenghui codelipenghui deleted the key_shared_ordering branch May 19, 2021 05:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Huanli-Meng Huanli-Meng Huanli-Meng left review comments

@jiazhai jiazhai jiazhai approved these changes

@sijie sijie Awaiting requested review from sijie

@merlimat merlimat Awaiting requested review from merlimat

Assignees

@codelipenghui codelipenghui

Labels

area/broker

Projects

None yet

Milestone

2.6.0

Development

Successfully merging this pull request may close these issues.

Dispatch messages to the new consumer after exist consumer acknowledged all dispatched messages in Key_Shared Subscription.

5 participants

@codelipenghui @merlimat @Huanli-Meng @feeblefakie @jiazhai