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

[stats] Add Key_Shared metadata to topic stats #11839

Merged
merged 3 commits into from
Oct 5, 2021
Merged

[stats] Add Key_Shared metadata to topic stats #11839

merged 3 commits into from
Oct 5, 2021

Conversation

massakam
Copy link
Contributor

@massakam massakam commented Aug 30, 2021 •
edited

Motivation

Users can specify the Key_Shared subscription policy as follows:

Consumer<byte[]> consumer = pulsarClient.newConsumer()
        .topic("persistent://public/default/t1")
        .subscriptionName("sub1")
        .subscriptionType(SubscriptionType.Key_Shared)
        .keySharedPolicy(KeySharedPolicy.autoSplitHashRange().setAllowOutOfOrderDelivery(true))
        .subscribe();

However, these metadata are not currently included in the topic stats.

Modifications

Added the following two fields to the topic stats:

  • allowOutOfOrderDelivery
  • keySharedMode

Verifying this change

  • Make sure that the change passes the CI checks.

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)

@massakam massakam added type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages doc Your PR contains doc changes, no matter whether the changes are in markdown or code files. component/stats labels Aug 30, 2021
@massakam massakam added this to the 2.9.0 milestone Aug 30, 2021
@massakam massakam self-assigned this Aug 30, 2021
merlimat
merlimat reviewed Aug 30, 2021
View reviewed changes
...common/src/main/java/org/apache/pulsar/common/policies/data/stats/SubscriptionStatsImpl.java
@@ -103,6 +103,12 @@
/** Mark that the subscription state is kept in sync across different regions. */
public boolean isReplicated;

/** Whether out of order delivery is allowed on the Key_Shared subscription. */
public boolean allowOutOfOrderDelivery;
Copy link
Contributor

Choose a reason for hiding this comment

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

This should also be added to the SubscriptionStats interface.

Also, since this is specific to KeyShared subscriptions, it might get confusing when using other subscription types. We could instead introduce a sub-interface/sub-object to show the key-shared policy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@merlimat

Also, since this is specific to KeyShared subscriptions, it might get confusing when using other subscription types. We could instead introduce a sub-interface/sub-object to show the key-shared policy.

Does that mean we should add the following two?

  • KeySharedSubscriptionStats interface that extends the SubscriptionStats interface
  • KeySharedSubscriptionStatsImpl class that extends the SubscriptionStatsImpl class and implements the KeySharedSubscriptionStats interface

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually added the above sub-interface/sub-class, but then the deserialization of the json response didn't seem to work on the pulsar-client-admin side. So I just added getters to the SubscriptionStats interface.

@Anonymitaet
Copy link
Member

@massakam thanks for adding docs!

@massakam
Copy link
Contributor Author

massakam commented Sep 1, 2021

PTAL

eolivelli
eolivelli approved these changes Sep 21, 2021
View reviewed changes
Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

+1

@nkurihar nkurihar merged commit 0180735 into apache:master Oct 5, 2021
@massakam massakam deleted the key-shared-stats branch October 5, 2021 06:25
codelipenghui pushed a commit that referenced this pull request Oct 6, 2021
@codelipenghui
[stats] Add Key_Shared metadata to topic stats (#11839)
ce634e2 
* Add Key_Shared metadata to topic stats

* Add getters to SubscriptionStats

* Resolve conflicts

(cherry picked from commit 0180735)
@codelipenghui codelipenghui added the cherry-picked/branch-2.8 Archived: 2.8 is end of life label Oct 6, 2021
bharanic-dev pushed a commit to bharanic-dev/pulsar that referenced this pull request Mar 18, 2022
@ciaocloud
[stats] Add Key_Shared metadata to topic stats (apache#11839)
0089c40 
* Add Key_Shared metadata to topic stats

* Add getters to SubscriptionStats

* Resolve conflicts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eolivelli eolivelli eolivelli approved these changes

@codelipenghui codelipenghui codelipenghui approved these changes

@nkurihar nkurihar nkurihar approved these changes

@merlimat merlimat Awaiting requested review from merlimat

Assignees

@massakam massakam

Labels
cherry-picked/branch-2.8 Archived: 2.8 is end of life doc Your PR contains doc changes, no matter whether the changes are in markdown or code files. release/2.8.2 release/2.9.0 type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages
Projects
None yet
Milestone
2.9.0
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@massakam @Anonymitaet @merlimat @eolivelli @codelipenghui @nkurihar