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.

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

PIP-119: Enable consistent hashing by default on KeyShared subscriptions dispatcher #13352

Merged
merged 3 commits into from
Jan 25, 2022
Merged

PIP-119: Enable consistent hashing by default on KeyShared subscriptions dispatcher #13352

merged 3 commits into from
Jan 25, 2022

Conversation

HQebupt
Copy link
Contributor

@HQebupt HQebupt commented Dec 16, 2021 •
edited by Anonymitaet
Loading

Fixed #13305

Motivation

See #13305

Modifications

Change subscriptionKeySharedUseConsistentHashing default value from false to true in the broker.conf.

Verifying this change

  • Make sure that the change passes the CI checks.

This change is a trivial rework / code cleanup without any test coverage.

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 and label this PR (if you have committer privilege).

Need to update docs?

  • doc

@github-actions
Copy link

@HQebupt:Thanks for your contribution. For this PR, do we need to update docs?
(The PR template contains info about doc, which helps others know more about the changes. Can you provide doc-related info in this and future PR descriptions? Thanks)

@Anonymitaet Anonymitaet added doc Your PR contains doc changes, no matter whether the changes are in markdown or code files. and removed doc-label-missing labels Dec 16, 2021
@Anonymitaet
Copy link
Member

Hi @HQebupt When submitting a PR, please provide doc related info in the PR description by ticking the box or labeling a PR directly, so that Bot will recognize the info and then label the PR correctly, or else Bot can not recognize the info and then label the PR with the doc-info-missing label.

Just now I re-label this PR and tick the box, please pay attention next time, thanks.

@HQebupt
Copy link
Contributor Author

HQebupt commented Dec 16, 2021

/pulsarbot run-failure-checks

@HQebupt
Copy link
Contributor Author

HQebupt commented Dec 17, 2021

/pulsarbot run-failure-checks

2 similar comments
@HQebupt
Copy link
Contributor Author

HQebupt commented Dec 17, 2021

/pulsarbot run-failure-checks

@HQebupt
Copy link
Contributor Author

HQebupt commented Dec 18, 2021

/pulsarbot run-failure-checks

@HQebupt
Copy link
Contributor Author

HQebupt commented Jan 18, 2022

PIP-119 will release at 2.10.

@merlimat merlimat added this to the 2.10.0 milestone Jan 22, 2022
@merlimat merlimat added release/note-required type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages labels Jan 22, 2022
@HQebupt
Copy link
Contributor Author

HQebupt commented Jan 22, 2022

Thanks @merlimat

@HQebupt
Copy link
Contributor Author

HQebupt commented Jan 23, 2022

/pulsarbot run-failure-checks

1 similar comment
@HQebupt
Copy link
Contributor Author

HQebupt commented Jan 23, 2022

/pulsarbot run-failure-checks

@merlimat merlimat merged commit 5b39913 into apache:master Jan 25, 2022
@BewareMyPower BewareMyPower mentioned this pull request Jan 26, 2022
Merged
1 task
@lhotari
Copy link
Member

lhotari commented Jan 26, 2022 •
edited
Loading

This change seems to cause problems. It seems to lead to message loss.

See #13963 for more details:
"I tried to use three Java consumers with Key_Shared subscription to consume the topic produced by C++ test KeySharedConsumerTest.testMultiTopics. Sometimes not all messages can be received as well. It looks like there is something wrong with the consistent hashing implementation of Key_Shared dispatcher."

@lhotari
Copy link
Member

lhotari commented Jan 26, 2022

I filed an issue #13965 since there seems to be a real issue and not just a flaky test. @HQebupt do you have a chance to investigate it?

@HQebupt
Copy link
Contributor Author

HQebupt commented Jan 26, 2022

I filed an issue #13965 since there seems to be a real issue and not just a flaky test. @HQebupt do you have a chance to investigate it?

Sure. I'll take a look.

@eolivelli
Copy link
Contributor

we still have this problem to be fixed @HQebupt @merlimat
#10750

This was referenced Feb 28, 2023
Closed
Closed
BewareMyPower added a commit to BewareMyPower/pulsar-client-go that referenced this pull request Feb 28, 2023
@BewareMyPower
Fix flaky Key_Shared subscription related tests
0eb844f 
### Motivation

After apache#953, the Pulsar
version was upgraded from 2.8.3 to 2.10.3. However,
[PIP-119](apache/pulsar#13352) changed the
default value of `subscriptionKeySharedUseConsistentHashing` to true,
which leads to the flakiness of Key_Shared subscription related tests.

Example: https://github.com/apache/pulsar-client-go/actions/runs/4291098473/jobs/7475868787

### Modifications

Configure `subscriptionKeySharedUseConsistentHashing` with `false`.
@BewareMyPower BewareMyPower mentioned this pull request Feb 28, 2023
Merged
shibd pushed a commit to apache/pulsar-client-go that referenced this pull request Mar 1, 2023
@BewareMyPower
Fix flaky Key_Shared subscription related tests (#970)
d60bff6 
### Motivation

After #953, the Pulsar
version was upgraded from 2.8.3 to 2.10.3. However,
[PIP-119](apache/pulsar#13352) changed the
default value of `subscriptionKeySharedUseConsistentHashing` to true,
which leads to the flakiness of Key_Shared subscription related tests.

Example: https://github.com/apache/pulsar-client-go/actions/runs/4291098473/jobs/7475868787

### Modifications

Configure `subscriptionKeySharedUseConsistentHashing` with `false`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@codelipenghui codelipenghui codelipenghui approved these changes

@merlimat merlimat merlimat approved these changes

Assignees

@HQebupt HQebupt

Labels
doc Your PR contains doc changes, no matter whether the changes are in markdown or code files. type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages
Projects
None yet
Milestone
2.10.0
Development

Successfully merging this pull request may close these issues.

PIP-119: Enable consistent hashing by default on KeyShared dispatcher
6 participants
@HQebupt @Anonymitaet @lhotari @eolivelli @merlimat @codelipenghui