Skip to content

Conversation

@shibd
Copy link
Member

@shibd shibd commented Feb 7, 2025

Motivation

Documentation

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

Matching PR in forked repository

PR in forked repository:

@shibd shibd self-assigned this Feb 7, 2025
@shibd shibd added the type/PIP label Feb 7, 2025
@github-actions github-actions bot added doc-not-needed Your PR changes do not impact docs PIP labels Feb 7, 2025
omarkj pushed a commit to omarkj/pulsar that referenced this pull request Feb 7, 2025
…a schema and transaction

Discussion email: 
Implementation PR: [improve][pip] PIP-406: Introduce pulsar_subscription_dispatch_throttled_msgs and bytes metrics apache#23945
omarkj pushed a commit to omarkj/pulsar that referenced this pull request Feb 7, 2025
…a schema and transaction

Discussion email: 
Implementation PR: [improve][pip] PIP-406: Introduce pulsar_subscription_dispatch_throttled_msgs and bytes metrics apache#23945
@shibd shibd requested a review from poorbarcode February 11, 2025 10:59
@shibd shibd changed the title [improve][pip] PIP-406: Introduce pulsar_subscription_dispatch_throttled_msgs and bytes metrics [improve][pip] PIP-406: Introduce metrics related to dispatch_throttled_count Feb 18, 2025
@shibd shibd requested a review from RobertIndie February 18, 2025 09:03
@shibd
Copy link
Member Author

shibd commented Feb 18, 2025

hi, @RobertIndie @tjiuming @poorbarcode

The previous design made it difficult for users to identify where throttling was causing a high backlog. I have rewritten the PIP to separate the metrics at the broker, topic, and subscription levels. Please review it again.

Thanks.

@shibd shibd requested a review from lhotari February 26, 2025 07:52
Copy link
Member

@RobertIndie RobertIndie left a comment

Choose a reason for hiding this comment

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

I think we don't need separate metrics for both messages throttled and bytes throttled. This could make it complex for users. Could we just name it pulsar_dispatch_throttled_events and only count the number of throttled events where the message count or bytes exceed the rate limits?

So the logic would be like:

     private boolean applyDispatchRateLimitsToReadLimits(DispatchRateLimiter rateLimiter,
                                                         MutablePair<Integer, Long> readLimits,
                                                         DispatchRateLimiter.Type limiterType) {
+        int originalMessagesToRead = readLimits.getLeft();
+        long originalBytesToRead = readLimits.getRight();
         // update messagesToRead according to available dispatch rate limit.
         int availablePermitsOnMsg = (int) rateLimiter.getAvailableDispatchRateLimitOnMsg();
         if (availablePermitsOnMsg >= 0) {
@@ -414,6 +416,12 @@ private boolean applyDispatchRateLimitsToReadLimits(DispatchRateLimiter rateLimi
         if (availablePermitsOnByte >= 0) {
             readLimits.setRight(Math.min(readLimits.getRight(), availablePermitsOnByte));
         }
+        if (readLimits.getLeft() < originalMessagesToRead || readLimits.getRight() < originalBytesToRead) {
+            rateLimiter.increaseDispatchThrottleMsgCount();
+        }

@shibd shibd changed the title [improve][pip] PIP-406: Introduce metrics related to dispatch_throttled_count [improve][pip] PIP-406: Introduce metrics related to dispatch throttled events Mar 13, 2025
@shibd shibd requested a review from RobertIndie March 13, 2025 11:59
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, thank you @shibd

@shibd
Copy link
Member Author

shibd commented Mar 17, 2025

https://lists.apache.org/thread/pmgkf67ph98k5l3lr60rtg96octb397q

The vote passed with 3 +1 binding.

  • Lari
  • Zike
  • Yubiao

@shibd shibd merged commit 67df710 into apache:master Mar 17, 2025
24 checks passed
walkinggo pushed a commit to walkinggo/pulsar that referenced this pull request Oct 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

doc-not-needed Your PR changes do not impact docs PIP type/PIP

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants