ingester: reduce active series when series change owner #8084
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lamida
reviewed
May 9, 2024
lamida
reviewed
May 9, 2024
pr00se
approved these changes
May 15, 2024
CHANGELOG.md
Outdated
| @@ -32,6 +32,7 @@ | |||
| * [ENHANCEMENT] OTLP: Improve remote write format translation performance by using label set hashes for metric identifiers instead of string based ones. #8012 | |||
| * [ENHANCEMENT] Querying: Remove OpEmptyMatch from regex concatenations. #8012 | |||
| * [ENHANCEMENT] Store-gateway: add `-blocks-storage.bucket-store.max-concurrent-queue-timeout`. When set, queries at the store-gateway's query gate will not wait longer than that to execute. If a query reaches the wait timeout, then the querier will retry the blocks on a different store-gateway. If all store-gateways are unavailable, then the query will fail with `err-mimir-store-consistency-check-failed`. #7777 | |||
| * [ENHANCEMENT] Ingester: active series are now updated along with owned series. They decrease when series change ownership between ingesters. This helps provide a more accurate total of active series when ingesters are added. #8084 | |||
There was a problem hiding this comment.
Should we specifically call out here that this is only enabled if the owned series flags (ingester.track-ingester-owned-series or ingester.use-ingester-owned-series-for-limits) are enabled?
Maybe it's enough to change it to "when owned series are enabled, active series are now..."?
There was a problem hiding this comment.
I mentioned the two flags. This makes the changelog a bit more self-sufficient to digest new changes
|
|
||
| // since no reason set, shard size and local limit are unchanged, and we pass ringChanged=false, no recompute will happen | ||
| c.updateOwnedSeriesAndCheckResult(t, false, 0, "") | ||
| c.checkTestedIngesterOwnedSeriesState(t, ownedServiceSeriesCount, 1, ownedServiceTestUserSeriesLimit) | ||
| c.checkUpdateReasonForUser(t, "") | ||
| c.checkActiveSeriesCount(t, ownedServiceSeriesCount) | ||
|
|
||
| // passing ringChanged=true will trigger recompute because the token ownership has changed | ||
| c.updateOwnedSeriesAndCheckResult(t, true, 1, recomputeOwnedSeriesReasonRingChanged) |
There was a problem hiding this comment.
should we check that active series go to 0 in this block?
There was a problem hiding this comment.
nice catch; the assertion was already in the partitions test case, but I had missed it for the classic ring. See 5acfce5
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
Co-authored-by: Jon Kartago Lamida <jon.lamida@grafana.com>
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
dimitarvdimitrov
force-pushed
the
dimitar/ingester/owned-active-series-ring-update-loop
branch
from
e40be24
to
75e3ece
Compare
dimitarvdimitrov
deleted the
dimitar/ingester/owned-active-series-ring-update-loop
branch
francoposa
pushed a commit
that referenced
this pull request
May 27, 2024
* calculate owned active series in owned series loop Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com> * Support clearing active series Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com> * Update owned series tests Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com> * Add test for early head compaction Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com> * Add CHANGELOG.md entry Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com> * Apply suggestions from code review Co-authored-by: Jon Kartago Lamida <jon.lamida@grafana.com> * Clarify CHANGELOG.md Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com> * Add missing assertion Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com> * Fix typo Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com> --------- Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com> Co-authored-by: Jon Kartago Lamida <jon.lamida@grafana.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What this PR does
Couples updates to owned series with updates to active series. If a series is no longer owned, it is no longer counted as an active series. The exception to that is during early head compaction: the series are still active even if they are removed from the head.
Which issue(s) this PR fixes or relates to
Fixes #7976
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX].about-versioning.mdupdated with experimental features.