-
Notifications
You must be signed in to change notification settings - Fork 544
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
Migration to dskit/ring #336
Conversation
As a side note, in dskit we should update metrics in the start fn and then start the ticker rather than do our first collect in 10 seconds, I'll create a PR for this tomorrow. grafana/dskit#61 |
Is this duplicate of #333? |
@pstibrany no, I think Tyler's PR is preferable :) |
Oh, that's too bad... you won the PR number lottery here :) |
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 will find many "Remove this newline, please." comments (at some point I've stopped reporting it, but please fix it everywhere). I would suggest you to check your goformat/goimports settings. Your IDE should do it for you.
Could you also rebase main
please?
Signed-off-by: Arve Knudsen <[email protected]>
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.
Generally looks good to me, but see comments please.
…tCompactor_ShouldFailCompactionOnTimeout
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.
Good job! I think I've just noticed an issue: could you take a look, 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.
Thanks!
// metrics := initialRegistries.BuildMetricFamiliesPerUser() | ||
// return metrics.GetSumOfGauges("cortex_ring_members") | ||
// }) | ||
dstest.Poll(t, 11*time.Second, float64(numAllGateways*numInitialGateways), func() interface{} { |
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 change from 5 seconds to 11 is worth calling out in the commit message.
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.
And should be rendered unnecessary by grafana/dskit#107, I think.
What this PR does: With including the ring package in dskit we want to migrate mimir and other users to the dskit.
Remaining work
TestStoreGateway_BlocksSyncWithDefaultSharding_RingTopologyChangedAfterScaleUp
not able to find ring members metricWhich issue(s) this PR fixes:
Fixes #
Checklist
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]