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.

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

Add cluster-aware quota limits for concurrent long-running requests #4662

Closed
wants to merge 19 commits into from

Conversation

MichaelSnowden
Copy link
Contributor

What changed?
I added some cluster-aware limits on the number of concurrent long-running requests and namespace-replication-inducing requests. I also did a lot of refactoring and added a bunch of documentation around here to make this clearer because the "namespace count" terminology was not very descriptive.

Why?
These limits are designed to protect database resources, so if we just use the per-instance limits, we could end up with a total limit which is very high when there are many instances, or, conversely, too low if there are few instances.

How did you test it?
I added unit tests.

Potential risks

Is hotfix candidate?

@MichaelSnowden MichaelSnowden requested a review from a team as a code owner July 20, 2023 23:05
@yux0
Copy link
Contributor

yux0 commented Jul 20, 2023

What is the difference between GlobalNamespaceReplicationInducingAPIsRPS and GlobalNamespaceNamespaceReplicationInducingAPIsRPS?

@MichaelSnowden
Copy link
Contributor Author

What is the difference between GlobalNamespaceReplicationInducingAPIsRPS and GlobalNamespaceNamespaceReplicationInducingAPIsRPS?

This was a mistake. There should only be one. I left the old one and removed the new one

@MichaelSnowden MichaelSnowden changed the title Add cluster-aware quota limits for concurrent long-running requests and replication-inducing requests Add cluster-aware quota limits for concurrent long-running requests Jul 21, 2023
Copy link
Member

@yycptt yycptt left a comment

Choose a reason for hiding this comment

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

I don't think ns count limit is for protecting DB resource. I am under the impression that those limit exists to protect resources like network connections, file descriptors on a host. But please correct me if I am wrong.

@MichaelSnowden
Copy link
Contributor Author

I don't think ns count limit is for protecting DB resource. I am under the impression that those limit exists to protect resources like network connections, file descriptors on a host. But please correct me if I am wrong.

This makes sense to me. I'm going to close this

@MichaelSnowden
Copy link
Contributor Author

I'm keeping the refactoring changes though and putting them in a separate PR here: #4665

@MichaelSnowden MichaelSnowden deleted the snowden/rate-limit branch August 8, 2023 18:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants