Fix: init of subcription rateLimiter to avoid race-condition and NPE

[rdhabalia]

Motivation

Initialize final dispatchRateLimiter to avoid multiple init for dispatchRateLimiter and avoid possible NPE due to uninitialized dispatchRateLimiter

[lovelle]

Excellent solution! just a question/proposal, has anyone ever considered of using annotations to document better thread safety like @ThreadSafe at a class level? A lot of people use jcip-annotations for this purpose and gives a clearer visibility about thread safety.

[rdhabalia]

@lovelle I think it makes more sense to use jcip-annotations for the public api for better documentation. for internally, I am not sure because most of the entities at broker and clients are thread-safe and but it might be helpful for developers if we come across with such apis that requires explicit documentation.

[lovelle]

Great! I agree 👍

[jiazhai]

@rdhabalia Thanks for the change. lgtm. but from the code, this looks like a lazy creation, which could avoid creating a dispatchRateLimiter if possible. @merlimat @sijie Would you please help confirm this?

[sijie]

I shared the same concern with @jiazhai - this change is forcing every consumer to have one instance of dispatcher limiter. this can be potentially expensive for users who have a lot of consumers. @rdhabalia can we find some other approaches to fix the problem?

[rdhabalia]

this change is forcing every consumer to have one instance of dispatcher limiter.

umm..that's not correct. Rate-Limiter is per dispatcher-level so, not every consumer will create a rate-limiter but it will be limited at subscription level. Also, this PR is not creating additional object but it just creates RateLimiter object in a thread-safe way.

But from the code, this looks like a lazy creation, which could avoid creating a dispatchRateLimiter if possible.

Actually existing code is also initializing rate-limiter as soon as subscription reads the message. It's lazy, non-thread-safe and unnecessary if throttling is not configured for the subscription: https://github.com/apache/pulsar/blob/master/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherSingleActiveConsumer.java#L373

However, we can avoid creating rateLimiter object if throttling is not configured and it can save us some heap. I will make the fix.

[jiazhai]

@rdhabalia Thanks for the fix in #3479. Does this mean we could close this pr?

[rdhabalia]

@jiazhai yes, we can close this one now.