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

Not allow use acknowledgeCumulative on Key_shared subscription. #5339

Merged
merged 3 commits into from
Oct 10, 2019

Conversation

codelipenghui
Copy link
Contributor

Motivation

Key_Shared subscription is a enhancement of shared subscription, we should avoid use acknowledgeCumulative on Key_shared subscription

Modifications

Add allow acknowledgeCumulative check for Key_shared subscription

Verifying this change

Add new unit test.

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)

@codelipenghui codelipenghui self-assigned this Oct 8, 2019
@codelipenghui codelipenghui added this to the 2.5.0 milestone Oct 8, 2019
@codelipenghui
Copy link
Contributor Author

run integration tests

Copy link
Contributor

@merlimat merlimat left a comment

Choose a reason for hiding this comment

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

LGTM. Also please add the same for C++ client

Message<Integer> message = consumer.receive();
if (i == 9) {
try {
consumer.acknowledgeCumulative(message);
Copy link
Contributor

Choose a reason for hiding this comment

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

add fail("should have failed") after calling ack, to verify the call is indeed throwing exception

if (i == 9) {
try {
consumer.acknowledgeCumulative(message);
} catch (PulsarClientException e) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be simplified into } catch (PulsarClientException.InvalidConfigurationException e) {

@codelipenghui
Copy link
Contributor Author

run Integration Tests

Copy link
Contributor

@merlimat merlimat left a comment

Choose a reason for hiding this comment

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

👍

@merlimat merlimat merged commit daa546d into apache:master Oct 10, 2019
merlimat pushed a commit to merlimat/pulsar that referenced this pull request Jan 17, 2020
…he#5339)

* Not allow use acknowledgeCumulative on Key_shared subscription.

* fix comments

* C++ Client add acknowledgeCumulative not allowed check.
@codelipenghui codelipenghui deleted the ack_cumulative_key_shared branch May 19, 2021 05:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants