Skip to content

Conversation

codesome
Copy link
Contributor

@codesome codesome commented Jun 2, 2020

What this PR does:

Adds series limit for TSDB blocks based ingester. Requires #2674

TODO

  • Unit tests
  • Handle these error where app.Add and app.AddFast is called - consider them to be soft errors
  • What to do when there is no metric name?
  • Update CHANGELOG.md

@codesome codesome changed the title Blocks series limits temp Series limit for TSDB blocks based ingester Jun 2, 2020
Copy link
Contributor

@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.

Thanks @codesome for working on this! A part from the feedback left on #2674 I would be glad to see these changes covered by tests. Could you check out any existing test on limits (we should have something for chunks storage) and replicate them for the blocks storage too, please? Copying these tests (instead of trying to reuse them) is fine to me.

Ganesh Vernekar added 3 commits June 3, 2020 13:36
Signed-off-by: Ganesh Vernekar <[email protected]>
Signed-off-by: Ganesh Vernekar <[email protected]>
@codesome codesome force-pushed the blocks-series-limits-temp branch from 339936b to b934641 Compare June 3, 2020 12:15
Signed-off-by: Ganesh Vernekar <[email protected]>
@codesome codesome force-pushed the blocks-series-limits-temp branch from b934641 to 5c0071a Compare June 3, 2020 12:17
@codesome codesome marked this pull request as ready for review June 3, 2020 12:17
@codesome
Copy link
Contributor Author

codesome commented Jun 3, 2020

This PR is now ready for review. I was able to re-use the existing tests to add case for the ingester v2. I need help in deciding the desired action for metric name limit when there is no metric name in a series.

Copy link
Contributor

@pstibrany pstibrany left a comment

Choose a reason for hiding this comment

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

Great job! Thank you Ganesh!

Signed-off-by: Ganesh Vernekar <[email protected]>
Copy link
Contributor

@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.

Good job! I left few nits, but overall looks good. I'm now going to review tests and do some manual tests.

@codesome codesome force-pushed the blocks-series-limits-temp branch from 9db2385 to 6ff8418 Compare June 5, 2020 10:13
@codesome codesome force-pushed the blocks-series-limits-temp branch from 175ee35 to 0a3a9db Compare June 5, 2020 10:22
Copy link
Contributor

@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.

Thanks Ganesh to address my comments! I've also done some manual tests locally and worked fine. I tested both per-user and per-metric limit and made sure that counters get correctly decremented when head is compacted.

Would mind doing a simple function renaming for clarity, please? I would rename:

  • seriesAddedFor() > increaseSeriesForMetric()
  • removeMetricName() > decreaseSeriesForMetric()

Also, if I remember correctly the changes in TSDB, callback doesn't guarantee atomicity. If this is correct, we should improve a bit removeMetricName() and handle the case shard.m[metricName] does not exist?

@pracucci
Copy link
Contributor

pracucci commented Jun 5, 2020

Also, if I remember correctly the changes in TSDB, callback doesn't guarantee atomicity. If this is correct, we should improve a bit removeMetricName() and handle the case shard.m[metricName] does not exist?

Discussed offline. Not an issue.

Signed-off-by: Ganesh Vernekar <[email protected]>
@codesome codesome force-pushed the blocks-series-limits-temp branch from 0a3a9db to f491c7b Compare June 5, 2020 12:19
@pull-request-size pull-request-size bot added size/XL and removed size/L labels Jun 5, 2020
@pracucci pracucci merged commit 1406f60 into cortexproject:master Jun 5, 2020
@pracucci pracucci mentioned this pull request Jul 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants