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

Global JetStream API request queue limiting #5900

Merged
merged 1 commit into from
Sep 19, 2024
Merged

Conversation

neilalexander
Copy link
Member

@neilalexander neilalexander commented Sep 17, 2024

This should provide some protection against the server being completely overwhelmed by a build-up of JS API requests.

Signed-off-by: Neil Twigg [email protected]

@neilalexander neilalexander changed the base branch from release/v2.10.21 to main September 19, 2024 11:24
@neilalexander neilalexander force-pushed the neil/jsrate210 branch 5 times, most recently from 438dbc2 to f2ee47d Compare September 19, 2024 14:18
@neilalexander neilalexander force-pushed the neil/jsrate210 branch 3 times, most recently from 8d16b37 to bd2e80e Compare September 19, 2024 15:58
@neilalexander neilalexander marked this pull request as ready for review September 19, 2024 16:32
@neilalexander neilalexander requested a review from a team as a code owner September 19, 2024 16:32
@derekcollison derekcollison self-requested a review September 19, 2024 16:47
pending, _ := s.jsAPIRoutedReqs.push(&jsAPIRoutedReq{jsub, sub, acc, subject, reply, copyBytes(rmsg), c.pa})
if pending >= warnThresh {
s.rateLimitFormatWarnf("JetStream request queue has high pending count: %d", pending)
limit := atomic.LoadInt64(&js.queueLimit)
Copy link
Member

Choose a reason for hiding this comment

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

One trick here we could do is that we know hard limit is greater them warn limit, so only if we hit warn limit do we look up limit with atomic and check hard limit.

Copy link
Member

Choose a reason for hiding this comment

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

We can do as a followup once we figure out test approach.

@derekcollison derekcollison self-requested a review September 19, 2024 17:28
Copy link
Member

@derekcollison derekcollison left a comment

Choose a reason for hiding this comment

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

LGTM

@derekcollison derekcollison merged commit 9870b6a into main Sep 19, 2024
5 checks passed
@derekcollison derekcollison deleted the neil/jsrate210 branch September 19, 2024 18:55
neilalexander added a commit that referenced this pull request Sep 20, 2024
Includes the following:

- #5901
- #5904
- #5900
- #5906
- #5908
- #5907

Signed-off-by: Neil Twigg <[email protected]>
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.

3 participants