-
Notifications
You must be signed in to change notification settings - Fork 60
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
Simplify SpreadMinimizingTokenGenerator for easier reuse in partitions ring. #462
Conversation
allTokens, err := t.generateAllTokens() | ||
if err != nil { | ||
// we were unable to generate required tokens, so we panic. | ||
panic(err) |
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.
This panic
is not new, it was just moved from generateTokensByInstanceID
to this method.
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
Before merging could you please check the comment I wrote?
} | ||
|
||
func (t *SpreadMinimizingTokenGenerator) CanJoin(instances map[string]InstanceDesc) error { | ||
if !t.canJoinEnabled { | ||
return nil | ||
} | ||
|
||
prevInstance, err := previousInstance(t.instance) | ||
if err != nil { | ||
if errors.Is(err, errorNoPreviousInstance) { |
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.
Could you please check whether we still need errorNoPreviousInstance
? I think that at this point it should be removed.
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.
This PR removed errorNoPreviousInstance
variable.
What this PR does:
This PR simplifies internals of
SpreadMinimizingTokenGenerator
: it removes logger, slice with zones, parsing of instance when computing previous instance.These changes make it easier to reuse this token generator in partitions ring code (https://github.com/grafana/dskit/compare/partitions-ring branch).
Checklist
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]