temporal icon indicating copy to clipboard operation
temporal copied to clipboard

Different error message for per namespace per shard rate limiting

Open prathyushpv opened this issue 1 year ago • 1 comments

What changed?

Different error message for per-shard per-namespace rate limiting. Shard level rate limiting was done by namespaceRateLimiter object. namespaceRatelimiter was a MultiRequestRateLimiter with both namespace rate limiter and shard-level rate limiter. Here it is split into two so we can differentiate between rate limiters and return appropriate error messages.

Why?

Currently, per-namespace rate limiter and per-shard per-namespace rate limiter return the same error message. It is difficult to pinpoint the reason for rate limiting in this case.

How did you test it?

Unit tests

Potential risks

None

Documentation

Is hotfix candidate?

No

prathyushpv avatar Aug 22 '24 22:08 prathyushpv

The changes look good to me, but it seems like more is changing here than just the error message distinction. If I'm understanding correctly, it looks to me like this PR also adds shard-level rate limiting to lots of (all?) persistence APIs. If that's the case, imo the title and description of the PR should represent both changes, so that when we're trying to skim through commits in the future, we know that this PR added shard-level rate limiting to persistence APIs and made the error messages different for per namespace vs per-shard

Thanks for taking a look! Shard level rate limiting is already there for persistence APIs. But that was done by namespaceRateLimiter object. namespaceRatelimiter was a MultiRequestRateLimiter with both namespace rate limiter and shard-level rate limiter. In this PR I split it into two so that we can differentiate between rate limiters and return appropriate error messages. I have updated the PR description.

prathyushpv avatar Aug 23 '24 00:08 prathyushpv