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

Upgrade ETCD/GRPC #3884

Merged
merged 4 commits into from
Feb 26, 2021
Merged

Conversation

jtlisi
Copy link
Contributor

@jtlisi jtlisi commented Feb 25, 2021

What this PR does:

  • Upgrade ETCD dependency to the latest version
  • Upgrade the grpc dependency to v1.32.0
  • Rename etcd imports to match the new etcd module packages
  • Port the GRPC naming naming package that was deprecated to pkg/util/naming to ensure no changes to behavior are introduced. I may be wrong but I don't think upstream grpc library provides a resolver interface that meets our use case anymore and we may be better off rolling our own implementation. I am open to suggestion on whether porting is the right approach.

Which issue(s) this PR fixes:
Fixes #2015

Checklist

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

Signed-off-by: Jacob Lisi <jacob.t.lisi@gmail.com>
Signed-off-by: Jacob Lisi <jacob.t.lisi@gmail.com>
Signed-off-by: Jacob Lisi <jacob.t.lisi@gmail.com>
Copy link
Contributor

@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.

I'm good with the copy, especially considering this may reduce the blast radius of issues we may potentially introduce with this upgrade (gRPC and etcd). I just left a comment about package naming.

package naming

// Copied from https://github.com/grpc/grpc-go/tree/v1.29.x/naming.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think to copy all these files to pkg/util/grpc/naming to make it more clear it's about grpc?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, I'll move the package.

Signed-off-by: Jacob Lisi <jacob.t.lisi@gmail.com>
@jtlisi jtlisi merged commit 35fbe42 into cortexproject:master Feb 26, 2021
@jtlisi jtlisi deleted the 20210224_upgrade_etcd_grpc branch February 26, 2021 21:59
@bboreham
Copy link
Contributor

bboreham commented Mar 5, 2021

This brings in at least one optimisation from gRPC: grpc/grpc-go#3568 (should be visible in streaming queries, also in hand-over from one ingester to another which I for one don't run any more).

Please look out for any differences visible in your dashboards, and consider including in CHANGELOG.

@pracucci pracucci mentioned this pull request Mar 5, 2021
3 tasks
kvrhdn added a commit to kvrhdn/tempo that referenced this pull request Jul 12, 2021
kvrhdn added a commit to kvrhdn/tempo that referenced this pull request Jul 13, 2021
kvrhdn added a commit to kvrhdn/tempo that referenced this pull request Jul 15, 2021
joe-elliott pushed a commit to grafana/tempo that referenced this pull request Jul 15, 2021
* Upgrade Cortex to latest release v1.9.0

* Don't set CompressResponses as this is a no-op

CompressResponses has been removed in v1.9.0 of Cortex. Additionally, this setting actually did not change anything when used in Tempo.

In Cortex this setting was used when creating the handler in query-frontend:
https://github.com/cortexproject/cortex/blob/4afaa357469fb37fd92cb1e2a0c1d10cdddc5ecd/pkg/cortex/modules.go#L557-L559
Since we do not use this code and have our own initialisation code for query-frontend, this setting never changed anything.

* Update github.com/go-openapi, match with prometheus/alertmanager

* Bump golangci-lint

* Check in all vendor files (some were ignored for some reason?)

* Update GRPC as ETCD has been updated in Cortex

ETCD has been updated here: cortexproject/cortex#3884

* make vendor-check

* Drop unnecessary replace statement
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix deprecated gRPC naming.Watcher
3 participants