[improve][broker] Pulsar Rate Limiting Refactoring changes (PIP-322) by lhotari · Pull Request #21681 · apache/pulsar

lhotari · 2023-12-06T16:45:48Z

Fixes #21442
PIP: #21680

Motivation

See PIP-322 for details.

Modifications

See PIP-322 for details.

Documentation

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

… volatile int field

…ishBufferSizeInMB

…actoring

merlimat

Great work here!

Can we also include the JMH benchmark from https://github.com/lhotari/async-tokenbucket/blob/master/src/jmh/java/com/github/lhotari/asynctokenbucket/AsyncTokenBucketBenchmark.java . It would be great to use it down the road for validating further changes to the implementation.

merlimat · 2023-12-14T18:41:07Z

+
+        @Override
+        public void close() {
+            closed = true;


Should we wait here until the internal thread is gone?

Eg:

We interrupt the thread

Use a barrier to wait on cloe

I'll switch to use Thread.interrupt()

merlimat · 2023-12-15T05:46:41Z

+    /**
+     * Interface for a clock source that returns a monotonic time in nanoseconds with a required precision.
+     */
+    public interface MonotonicClockSource {


I'd keep this in a separate file as it might be interesting for usages other than the rate limiter.

+1. The reason to put everything in a single file was to make AsyncTokenBucket self contained at first. I'm thinking of moving all AsyncTokenBucket files to a separate "qos" package.

One detail about this MonotonicClockSource interface is designed for AsyncTokenBucket's use case, so I'll need to find a way to express that.

Resolved by moving to separate file and renaming to MonotonicSnapshotClock

merlimat · 2023-12-15T05:47:52Z

+         *                      if false, the returned value can be a granular precision monotonic time in nanoseconds.
+         * @return the current monotonic clock time in nanoseconds
+         */
+        long getNanos(boolean highPrecision);


Would we have to mix high/low precision in the same rate limiter instance? Otherwise, we could just have different implementations, for different tradeoffs or scopes.

This is needed for AsyncTokenBucket's use case. I'll try to make it more understandable. The "highPrecision" is now perhaps causing the confusion. The use case in AsyncTokenBucket is with the usages where a consistent view of the token balance is requested.

I'll rename the parameter or add documentation to make it clear why there's both high/low precision in the same interface and method.

Update: I am renaming MonotonicClockSource to MonotonicSnapshotClock and making it explicit that it's a clock that snapshots the underlying high precision clock source periodically. I am renaming "highPrecision" to "requestSnapshot" to make it clear that when the flag is set, the snapshot is updated in the call before returning the value.

Done. PTAL.

merlimat · 2023-12-15T05:48:40Z

+         *                      if false, the returned value can be a granular precision monotonic time in nanoseconds.
+         * @return the current monotonic clock time in nanoseconds
+         */
+        long getNanos(boolean highPrecision);


nit: maybe rename to getCurrentTimeNanos() ?

+1, I'll rename it. getCurrentTickNanos perhaps. The reason for this is that time could be confusing since it's not the time of the day (like it is in System.currentTimeMillis(). I think tick might be a better description of what the returned value is.

Renaming to getTickNanos

merlimat · 2023-12-15T05:50:43Z

+     * Instantiating this class creates a daemon thread that updates the monotonic time. The close method
+     * should be called to stop the thread.
+     */
+    public static class GranularMonotonicClockSource implements MonotonicClockSource, AutoCloseable {


We can move to separate file

merlimat · 2023-12-15T05:52:04Z

+ * constructing higher-level asynchronous rate limiter implementations, which require side effects for throttling.
+ * <p>To achieve optimal performance, pass a {@link GranularMonotonicClockSource} as the clock source.
+ */
+public abstract class AsyncTokenBucket {


There are many classes/interfaces here. Let's move this to a dedicated package and have all classes in separate files.

…teLoop

…ar-broker

…actoring

…hrottling

…ng duration directly

lhotari · 2023-12-15T15:21:40Z

Great work here!

Can we also include the JMH benchmark from https://github.com/lhotari/async-tokenbucket/blob/master/src/jmh/java/com/github/lhotari/asynctokenbucket/AsyncTokenBucketBenchmark.java . It would be great to use it down the road for validating further changes to the implementation.

@merlimat Thanks for that review! Great idea! I added a module "microbench". There's also a README.md file that explains how to compile and run the JMH benchmarks.
I have now addressed your review comments. PTAL

nodece · 2024-01-31T12:34:06Z

This PR looks to break the resourcegroup rate limiter, please see the Prometheus graph:

When using the 3.3.0 version, the pulsar_rate_in metric is not as smooth as the 3.0.1 version.

The following is the broker.conf:

PULSAR_PREFIX_resourceUsageTransportClassName=org.apache.pulsar.broker.resourcegroup.ResourceUsageTopicTransportManager

Test:

bin/pulsar-admin resourcegroups create rg-test-1 -mp 200
bin/pulsar-admin namespaces set-resource-group public/default -rgn rg-test-1
bin/pulsar-perf produce rg-test-1 --disable-batching -r 600 --size 10

Could you help to check out this issue?

### Motivation [Pulsar#21681](apache/pulsar#21681) introduced new API changes, which caused a compatibility issue. ### Modifications Use `incrementPublishCount(producer, numMessages, numBytes);` to set the Rate Limiting required counts.

lhotari added 30 commits December 6, 2023 07:28

Add AtomicIntegerFlags for handling multiple atomic flags in a single…

9ca636c

… volatile int field

Start working on ThrottleTracker

e982a14

Add AsyncTokenBucket

c94fa36

Start working on ServerCnxThrottleTracker

c08af16

Apply changes made by Chris Bono

37b549b

Handle pendingSendRequest and per-thread publish limit maxMessagePubl…

d07d370

…ishBufferSizeInMB

increase publish limited times for topics

3f6d841

remove enableCnxAutoRead / disableCnxAutoRead

3963a71

Remove throttledConnectionsGlobal that is never incremented

bf60780

Rename method

1d0cff3

Cleanup

29cf5f2

Get rid of tryAcquire

f8c94e3

Refactor PublishRateLimiter interface

7ce1836

Rename method

f66a113

Remove

7faa542

Replace with Guava RateLimiter

a10a2a3

Remove Pulsar RateLimiter

dfecdc0

Remove PrecisePublishLimiter

9bf2862

Update test

849aed7

Remove PublishRateLimiterMonitor

4418b8d

checkstyle

aaa5412

Continue removing unnecessary methods

5681500

Remove / comment-out old code

fc488b2

Remove more dead code

d44064d

Spike on AsyncTokenBucket usage in PublishRateLimiterImpl

def1df1

Introduce ThrottleHandler

a65d69e

Rename

535b552

Improve ThrottleHandler

effe933

Remove ThrottleHandler, simply return a value object

74c72d3

Remove PublishRateLimiterDisable

cf53f5e

lhotari marked this pull request as ready for review December 11, 2023 20:31

lhotari requested a review from Shawyeok December 11, 2023 20:33

lhotari marked this pull request as draft December 12, 2023 12:18

lhotari added 2 commits December 13, 2023 18:22

Refactor AsyncTokenBucket based on review feedback

af9f576

Merge remote-tracking branch 'origin/master' into lh-rate-limiter-ref…

302bbda

…actoring

lhotari marked this pull request as ready for review December 13, 2023 16:42

lhotari marked this pull request as draft December 13, 2023 16:54

lhotari marked this pull request as ready for review December 14, 2023 15:12

merlimat approved these changes Dec 15, 2023

View reviewed changes

lhotari added 13 commits December 15, 2023 12:39

Address review feedback: use Thread.interrupt() to terminate the upda…

a443ca6

…teLoop

Move AsyncTokenBucket to org.apache.pulsar.broker.qos package in puls…

b72216a

…ar-broker

Split AsyncTokenBucket into individual source code files

4ded3cc

Rename MonotonicClockSource to MonotonicSnapshotClock

0a30cb7

Read the volatile field again to ensure monotonicity

8a86437

Polish

5a0c6be

clockSource -> clock

45f7c47

More monotonic snapshot clock renamings

f397a98

Merge remote-tracking branch 'origin/master' into lh-rate-limiter-ref…

208f34d

…actoring

Fix race condition in PublishRateLimiterImpl which could stop the unt…

52e3bc3

…hrottling

Eliminate the need to call containsTokens by calculating the throttli…

7ded71b

…ng duration directly

Add JMH benchmark

b499a61

Fix checkstyle error

324bb96

merlimat approved these changes Dec 15, 2023

View reviewed changes

merlimat merged commit c4cff0a into apache:master Dec 15, 2023

dragosvictor mentioned this pull request May 3, 2024

[improve][doc] Cleanup broker connection metrics documentation apache/pulsar-site#893

Merged

2 tasks

This was referenced May 5, 2025

[fix] chore: remove unused preciseTopicPublishRateLimiterEnable dlg99/pulsar#23

Open

[fix] chore: remove unused preciseTopicPublishRateLimiterEnable #24249

Merged

Conversation

lhotari commented Dec 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Modifications

Documentation

Uh oh!

merlimat left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lhotari Dec 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lhotari commented Dec 15, 2023

Uh oh!

nodece commented Jan 31, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

lhotari commented Dec 6, 2023 •

edited

Loading

lhotari Dec 15, 2023 •

edited

Loading