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

block-builder: Add basic series limit #9983

Merged
merged 5 commits into from
Nov 27, 2024
Merged

block-builder: Add basic series limit #9983

merged 5 commits into from
Nov 27, 2024

Conversation

codesome
Copy link
Member

What this PR does

This is a very naive solution to the limits before we have a better solution.

With this PR, we will allow up to the global series allowed for the user per partition, iff they global limit is under block-builder.apply-global-series-under. We do not apply any limits if the configured limit is over the flag value.

Few examples of it working:
Let's assume block-builder.apply-global-series-under=10000

  1. If a user has global limit of 5k series, and is sharded between 3 partitions, then it will allow 5k series per partition. So technically this user can ingest up to 15k series.
  2. If a user has global limit of 5k series, and is sharded only on 1 partition, then with 5k series per partition, the user will be limited to 5k series as intended.
  3. If a user has global limit of 15k series, and no matter how many partitions they are sharded to, while ingesters will apply the proper limits, block builder will not apply any limits (meaning they can ingest anything).

Checklist

  • Tests updated.
  • Documentation added.
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX].
  • about-versioning.md updated with experimental features.

@codesome codesome requested a review from a team as a code owner November 21, 2024 20:49
Copy link
Collaborator

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

I only checked the config to see the overall idea of this change (which LGTM). I let rest of the team reviewing the actual code changes.

pkg/blockbuilder/config.go Outdated Show resolved Hide resolved
Copy link
Contributor

@seizethedave seizethedave left a comment

Choose a reason for hiding this comment

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

LGTM modulo commentary 😸

}

userLimit := b.limits.MaxGlobalSeriesPerUser(userID)
if userLimit <= b.applyGlobalSeriesLimitUnder {
Copy link
Contributor

Choose a reason for hiding this comment

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

I realize this is a temporary measure, but Under doesn't match with <=. It's really an AtMost. Perhaps applyGlobalSeriesLimitMax?

Copy link
Member Author

Choose a reason for hiding this comment

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

With the reword to applyMaxGlobalSeriesPerUser..., using Max as a suffix again sounds odd. How about applyMaxGlobalSeriesPerUserBelow? And put in comments that it is inclusive.

pkg/blockbuilder/tsdb.go Show resolved Hide resolved
Signed-off-by: Ganesh Vernekar <[email protected]>
@codesome
Copy link
Member Author

@seizethedave I updated the PR. PTAL.

@codesome codesome enabled auto-merge (squash) November 27, 2024 20:16
@codesome codesome merged commit b941204 into main Nov 27, 2024
29 checks passed
@codesome codesome deleted the codesome/bb-limits branch November 27, 2024 20:21
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