-
Notifications
You must be signed in to change notification settings - Fork 529
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
Allow ingesters to configure SpreadMinimizingTokenGenerator #5308
Conversation
…gester-ring Signed-off-by: Yuri Nikolic <durica.nikolic@grafana.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I just left few nites.
pkg/ingester/ingester_ring.go
Outdated
case randomTokenGeneration: | ||
return ring.NewRandomTokenGenerator() | ||
default: | ||
warn := fmt.Sprintf("Unsupported token generation strategy (\"%s\") has been chosen for %s. RandomTokenGenerator will be used instead.", cfg.TokenGeneratorStrategy, tokenGenerationStrategyFlag) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nit] Validate the TokenGeneratorStrategy
configured value in RingConfig.Validate()
instead, so that it's consistent with the rest of the config validation. No need to log it there, just return the error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but see nits (and Marco's comments).
Signed-off-by: Yuri Nikolic <durica.nikolic@grafana.com>
e553475
to
46e6d70
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Yuri for addressing my feedback. A couple of last comments and we can go!
Signed-off-by: Yuri Nikolic <durica.nikolic@grafana.com>
Signed-off-by: Yuri Nikolic <durica.nikolic@grafana.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
What this PR does
This PR introduces 2 experimental CLI flags to
ingester.RingConfig
:token-generation-strategy
: specifies the strategy used for generating tokens for ingesters. Possible values arerandom-tokens
(default) andspread-min-tokens
.spread-minimizing-zones
: comma-separated list of zones in whichSpreadMinimizingTokenGenerator
is used for token generation. This configuration is used only whentoken-generation-strategy
is set tospread-min-tokens
.Which issue(s) this PR fixes or relates to
Part of #4736
Checklist
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]