-
Notifications
You must be signed in to change notification settings - Fork 476
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
Deprecate readiness-check-ring-health config option #4422
Conversation
263f19c
to
7aec58e
Compare
pkg/ingester/ingester_ring.go
Outdated
@@ -119,7 +125,7 @@ func (cfg *RingConfig) ToRingConfig() ring.Config { | |||
|
|||
// ToLifecyclerConfig returns a ring.LifecyclerConfig based on the ingester | |||
// ring config. | |||
func (cfg *RingConfig) ToLifecyclerConfig() ring.LifecyclerConfig { | |||
func (cfg *RingConfig) ToLifecyclerConfig(logger log.Logger) ring.LifecyclerConfig { |
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.
I have put the check in this method since it is where the flag is consumed, Another way is to implement a validate method of RingConfig, and call it from mimir.go
5999327
to
0f3ed8f
Compare
5dc37a8
to
302c04a
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 for working on this! I just left a couple of minor comments.
a8d6b78
to
9f4103b
Compare
The CHANGELOG has just been cut to prepare for the next Mimir release. Please rebase |
CHANGELOG.md
Outdated
@@ -4,6 +4,7 @@ | |||
|
|||
### Grafana Mimir | |||
|
|||
* [CHANGE] Ingester: the configuration parameter `-ingester.ring.readiness-check-ring-health` has been deprecated and will be removed in Mimir 2.9. #4422 |
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.
Contrary to what automatically commented, don't move it outside of 2.7 CHANGELOG because we want to cherry-pick this PR in release 2.7. Just rebase it, please.
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! Please don't move the CHANGELOG entry outside of 2.7 CHANGELOG because we want to cherry-pick this PR in release 2.7, but please rebase it so we can merge. Thanks!
9f4103b
to
083039e
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!
(cherry picked from commit 9e8e46a)
What this PR does
Which issue(s) this PR fixes or relates to
Fixes #4181
Checklist
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]