Skip to content
New issue

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

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

Already on GitHub? # to your account

[feat] Use producer name and sequence number as fallback key in Key_Shared implementation #23219

Merged

Conversation

pdolif
Copy link
Contributor

@pdolif pdolif commented Aug 26, 2024

Fixes #23212

Motivation

Taken from issue:

The Key_Shared implementation expects that the incoming message contain either a key/keyBytes or orderingKey.
If the key isn't set, the implementation will use the bytes of the NONE_KEY string as the key. This could be a surprise to users since the implications of not setting the key is not documented.

The problem that this causes is that all messages without a key will handled by a single consumer. If any of these messages is in redelivery, it will cause all message delivery to be blocked.

Modifications

  • Updated all parts where NONE_KEY was used as a default: Prefer 1) ordering key, 2) partition key, 3) new fallback key derived from producer name and sequence number, 4) NONE_KEY

Verifying this change

  • Make sure that the change passes the CI checks.

This change added tests and can be verified as follows:

  • Updated KeySharedSubscriptionTest to verify that messages are distributed evenly among consumers and based on the producer-derived key if no ordering/partition key is given
  • Added test case to CommandsTest for the new fallback key
  • Updated PerformanceProducerTest to reflect distribution of messages to multiple consumers if key is not set

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

If the box was checked, please highlight the changes

  • Dependencies (add or upgrade a dependency)
  • The public API
  • The schema
  • The default values of configurations
  • The threading model
  • The binary protocol
  • The REST endpoints
  • The admin CLI options
  • The metrics
  • Anything that affects deployment

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

Matching PR in forked repository

PR in forked repository: pdolif#2

Copy link

@phil-cd Please add the following content to your PR description and select a checkbox:

- [ ] `doc` <!-- Your PR contains doc changes -->
- [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
- [ ] `doc-not-needed` <!-- Your PR changes do not impact docs -->
- [ ] `doc-complete` <!-- Docs have been already added -->

@lhotari
Copy link
Member

lhotari commented Aug 26, 2024

Copy link
Member

@lhotari lhotari left a comment

Choose a reason for hiding this comment

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

LGTM. Great work @phil-cd ! Thanks for contributing.

@lhotari lhotari requested a review from eolivelli August 26, 2024 10:43
@pdolif
Copy link
Contributor Author

pdolif commented Aug 29, 2024

/pulsarbot rerun-failure-checks

@lhotari lhotari added this to the 4.0.0 milestone Oct 1, 2024
@lhotari lhotari merged commit 5b98d37 into apache:master Oct 1, 2024
60 of 61 checks passed
hanmz pushed a commit to hanmz/pulsar that referenced this pull request Feb 12, 2025
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
doc-required Your PR changes impact docs and you will update later. ready-to-test
Projects
None yet
2 participants