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

Allow ingesters to configure SpreadMinimizingTokenGenerator #5308

Merged
merged 4 commits into from
Jun 22, 2023

Conversation

duricanikolic
Copy link
Contributor

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 are random-tokens (default) and spread-min-tokens.
  • spread-minimizing-zones: comma-separated list of zones in which SpreadMinimizingTokenGenerator is used for token generation. This configuration is used only when token-generation-strategy is set to spread-min-tokens.

Which issue(s) this PR fixes or relates to

Part of #4736

Checklist

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

…gester-ring

Signed-off-by: Yuri Nikolic <durica.nikolic@grafana.com>
@duricanikolic duricanikolic requested review from a team as code owners June 22, 2023 11:02
@duricanikolic duricanikolic self-assigned this Jun 22, 2023
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.

LGTM. I just left few nites.

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)
Copy link
Collaborator

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.

pkg/ingester/ingester_ring.go Outdated Show resolved Hide resolved
pkg/ingester/ingester_ring.go Outdated Show resolved Hide resolved
pkg/ingester/ingester_ring.go Outdated Show resolved Hide resolved
pkg/ingester/ingester_ring.go Outdated Show resolved Hide resolved
pkg/ingester/ingester_ring.go Outdated Show resolved Hide resolved
pkg/ingester/ingester_ring.go Outdated Show resolved Hide resolved
Copy link
Contributor

@aknuds1 aknuds1 left a 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).

pkg/ingester/ingester_ring.go Outdated Show resolved Hide resolved
pkg/ingester/ingester_ring.go Outdated Show resolved Hide resolved
pkg/ingester/ingester_ring_test.go Outdated Show resolved Hide resolved
pkg/ingester/ingester_ring_test.go Outdated Show resolved Hide resolved
@aknuds1 aknuds1 added enhancement New feature or request component/ingester labels Jun 22, 2023
CHANGELOG.md Outdated Show resolved Hide resolved
Signed-off-by: Yuri Nikolic <durica.nikolic@grafana.com>
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.

Thanks Yuri for addressing my feedback. A couple of last comments and we can go!

CHANGELOG.md Outdated Show resolved Hide resolved
pkg/ingester/ingester_ring.go Outdated Show resolved Hide resolved
pkg/ingester/ingester_ring.go Outdated Show resolved Hide resolved
@aknuds1 aknuds1 changed the title Allow ingesters to configure SperadMinimizingTokenGenerator Allow ingesters to configure SpreadMinimizingTokenGenerator Jun 22, 2023
Signed-off-by: Yuri Nikolic <durica.nikolic@grafana.com>
Signed-off-by: Yuri Nikolic <durica.nikolic@grafana.com>
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.

LGTM, thanks!

@pracucci pracucci merged commit 06ed798 into main Jun 22, 2023
@pracucci pracucci deleted the yuri/spread-minimizing-tg branch June 22, 2023 15:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/ingester enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants