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

[feature][client] PIP-184: Topic specific consumer priorityLevel #16715

Merged
merged 2 commits into from
Aug 2, 2022

Conversation

nahguam
Copy link
Contributor

@nahguam nahguam commented Jul 21, 2022

Resolves #16481

Motivation

The Pulsar Java consumer supports setting a priority level for priority message
dispatch in shared subscription consumers and priority assignment in failover
subscription consumers. See the ConsumerBuilder.html#priorityLevel(int)
Javadoc
for a detailed functional description. The Pulsar Java consumer also supports
consuming from multiple topics. However, it is not possible to set a different
priority level for different topics in the same Consumer instance.

This behaviour is desirable in some use cases. For example, a consumer
processing region specific topics might wish to configure region stickiness - A
multi-region application might be consuming from topics events-us-east-1 and
events-eu-west-1. Consumers in all regions should be configured to consume all
topics to ensure data completeness. However, to ensure low latency, the
us-east-1 consumer would need to set a higher priority level for the us-east-1
topic. Similarly, the eu-west-1 consumer would need to set a higher priority
level for the eu-west-1 topic.

Without the ability to configure different priority levels for different
topics, the developer would have to create multiple consumer instances and have
to manage the consumption from them in their application code. This feature
provides developer convenience to keep application code the same regardless of
whether this is a requirement or not.

Modifications

Introduce new methods and configuration to support setting different priority levels for different topics in the same consumer instance.

Verifying this change

  • Make sure that the change passes the CI checks.

This change added tests and can be verified as follows:

(example:)

  • Unit test ConsumerImplTest.testTopicPriorityLevel() verifies the functionality.

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: (yes)
  • 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 or label this PR directly.

Need to update docs?

  • doc-required
    (Your PR needs to update docs and you will update later)

  • doc-not-needed
    (Please explain why)

  • doc
    (Your PR contains doc changes)

  • doc-complete
    (Docs have been already added)

@github-actions github-actions bot added the doc Your PR contains doc changes, no matter whether the changes are in markdown or code files. label Jul 21, 2022
@codelipenghui codelipenghui added this to the 2.11.0 milestone Jul 21, 2022
@codelipenghui codelipenghui added type/feature The PR added a new feature or issue requested a new feature component/client-java labels Jul 21, 2022
@codelipenghui codelipenghui modified the milestones: 2.11.0, 2.12.0 Jul 26, 2022
@Technoboy-
Copy link
Contributor

@nahguam Could you fix the checkstyle issue?

Copy link
Member

@mattisonchao mattisonchao left a comment

Choose a reason for hiding this comment

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

Look to me. I left some minor questions to hope you can answer.

Copy link
Member

@mattisonchao mattisonchao left a comment

Choose a reason for hiding this comment

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

I'm unsure if adding the test coverage to actual usage is necessary. I mean to create the consumer with different priorities and then consume messages from the broker. Not just test the client configuration.

@mattisonchao
Copy link
Member

@nahguam Could you please take a look failed test?

@nahguam
Copy link
Contributor Author

nahguam commented Jul 28, 2022

I'm unsure if adding the test coverage to actual usage is necessary. I mean to create the consumer with different priorities and then consume messages from the broker. Not just test the client configuration.

I would hope there are already tests to verify the priority level behaviour. This only changes how the priority level is wired in from the configuration. I added a test to ConsumerImplTest to verify the wiring.

@Technoboy-
Copy link
Contributor

Technoboy- commented Jul 29, 2022

@nahguam
There seems a failure test related to this change, could you help check it ?
https://github.com/apache/pulsar/runs/7571444716?check_suite_focus=true

@codelipenghui
Copy link
Contributor

The PR can only merge after the proposal is closed https://lists.apache.org/thread/08olgc0bjr13ld3g0hz58p1sj9c4pk98

@codelipenghui
Copy link
Contributor

@nahguam Please help fix the check style, so that we can merge this PR.

@Technoboy- Technoboy- merged commit cfd2178 into apache:master Aug 2, 2022
Gleiphir2769 pushed a commit to Gleiphir2769/pulsar that referenced this pull request Aug 4, 2022
@nahguam nahguam deleted the pip-topic-conf branch November 9, 2022 08:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/client doc Your PR contains doc changes, no matter whether the changes are in markdown or code files. type/feature The PR added a new feature or issue requested a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PIP-184: Topic specific consumer priorityLevel
5 participants