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

PIP-34 Add flag to enable or disable Key_Shared subscription. #4120

Merged
merged 3 commits into from
Apr 25, 2019

Conversation

codelipenghui
Copy link
Contributor

Motivation

Add a broker level flag to enable or disable Key_Shared subscription, disabled by default.

Modifications

Add a flag named subscriptionRedeliveryTrackerEnabled
Add documentation to describe Key_Shared subscription is a beta feature.

Verifying this change

This change is already covered by existing tests

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: (yes)
  • 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)

conf/broker.conf Outdated
@@ -107,6 +107,9 @@ subscriptionRedeliveryTrackerEnabled=true
# How frequently to proactively check and purge expired subscription
subscriptionExpiryCheckIntervalInMinutes=5

# Enable Key_Shared subscription (default is disabled)
subscriptionKeySharedEnable=false
Copy link
Contributor

Choose a reason for hiding this comment

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

I was just meaning to have it enabled by default (with an option to disable)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Enabled it by default and add a unit test to ensure disable is work well.

@codelipenghui codelipenghui requested a review from merlimat April 24, 2019 23:12
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.

👍 Thanks for adding it.

@@ -150,6 +150,8 @@ In *Key_Shared* mode, multiple consumers can attach to the same subscription. Me

![Key_Shared subscriptions](assets/pulsar-key-shared-subscriptions.png)

**Key_Shared subscription is a beta feature. You can disable it at broker.config.**
Copy link
Member

Choose a reason for hiding this comment

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

@codelipenghui shouldn't this be "You can enable it"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sijie Key_shared subscription is enabled by default. so i think we need to tell users can disable it at broker.config

Copy link
Member

Choose a reason for hiding this comment

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

There was a comments above to enable it by default.

Copy link
Member

Choose a reason for hiding this comment

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

Ah I see. sorry I missed that.

@sijie sijie merged commit 508fe22 into apache:master Apr 25, 2019
@codelipenghui codelipenghui deleted the key_shared_subscription_flag branch May 19, 2021 05:37
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.

4 participants