-
Notifications
You must be signed in to change notification settings - Fork 810
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
Upgrade dskit and pass empty service name to dskit/grpcutil.Resolver.Resolve #4601
Conversation
58913ca
to
eebac55
Compare
eebac55
to
79c0845
Compare
I think the integration tests might be running with an older version of Go, since |
Can we please update dskit to latest main, there are more incompatible changes (removal of ring keys from dskit), it would be best to tackle them all at once. WDYT? |
CHANGELOG.md
Outdated
@@ -76,7 +76,7 @@ | |||
* [BUGFIX] Querier: honor querier minT,maxT if `nil` SelectHints are passed to Select(). #4413 | |||
* [BUGFIX] Compactor: fixed panic while collecting Prometheus metrics. #4483 | |||
* [BUGFIX] Update go-kit package to fix spurious log messages #4544 | |||
|
|||
* [BUGFIX] Querier: Disable query scheduler SRV DNS lookup. #4601 |
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.
If I imagine myself as a Cortex admin, reading this, I don't think I would know whether it matters to me.
I think the effect was to reduce some noise in logs?
Looks like we also reduced some "connection refused" messages, and reworded some other log lines from the memberlist wrapper.
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.
@bboreham good point, I can try to improve the changelog entry. I'm not so familiar with the other changes in dskit, I guess I ought to try to summarize them also?
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've expanded upon this changelog entry, and added ones from dskit @pstibrany thought were useful.
@pstibrany is it best to tackle that also in this PR? I honestly am not sure how to tackle those changes. If it's easily done I don't mind. |
It's relatively easy to do, we just need to stop using constants from dskit, and define them in Cortex. I will give you a pointer via Slack. |
Thanks @pstibrany, coming up. |
142d6e5
to
a358047
Compare
@pstibrany upgraded to latest dskit as you asked. @bboreham wrote more on the changelog entry, plus added the ones @pstibrany thought were useful from the dskit changelog. |
fdb8a2e
to
ae64450
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.
Good job, thank you!
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
ae64450
to
1f32bad
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.
LGTM
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!
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com> Signed-off-by: Alvin Lin <alvinlin@amazon.com>
What this PR does:
Upgrade to dskit@01ce9286d7d5, which most importantly takes a new argument,
service
, todskit/grpcutil.Resolver.Resolve
. This argument configures the service part of the SRV DNS record to try and resolve. The PR passes an empty string for the aforementioned argument, so SRV lookup is disabled.Which issue(s) this PR fixes:
Checklist
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]