-
Notifications
You must be signed in to change notification settings - Fork 514
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
Gogoproto coded etcd #1260
Gogoproto coded etcd #1260
Conversation
pkg/gogocodec/gogocodec.go
Outdated
@@ -23,6 +23,7 @@ const ( | |||
jaegerProtoGenPkgPath = "github.com/jaegertracing/jaeger/proto-gen" | |||
jaegerModelPkgPath = "github.com/jaegertracing/jaeger/model" | |||
otelProtoPkgPath = "go.opentelemetry.io/collector" | |||
etcdProtoPkgPath = "go.etcd.io/etcd/api/v3/etcdserverpb" |
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.
nice fix, but i'm wondering if we can just update our etcd dependency instead of adding it here?
does the most recent etcd support latest grpc/proto?
also, we will definitely need to cut a 1.3.1 with this. good catch.
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.
The gogocodec hack for etcd can be removed once it's upgraded to grpc >v1.38 is released (tentatively next release from v3.5.1). We're stuck with this for a bit I'm afraid.
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.
FWIW, it's updated in main
, but still unreleased
cccf382
to
d021495
Compare
7ca0e06
to
022ded6
Compare
c85352f
to
a6dccd2
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.
Nice work, LGTM! 🚀
// wait for active ingesters | ||
time.Sleep(1 * time.Second) | ||
matchers := []*labels.Matcher{ | ||
func TestMicroservicesWithKVStores(t *testing.T) { |
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.
fantastic addition 👍
* Register etcd's server proto path in gogoproto codec * Upgrade etcd api patch version and add changelog entry * Extend e2e to test all kv stores * Serverles go mod tidy * Typo * Config hostname from suite * Remove docker namespace from hostname
* Register etcd's server proto path in gogoproto codec * Upgrade etcd api patch version and add changelog entry * Extend e2e to test all kv stores * Serverles go mod tidy * Typo * Config hostname from suite * Remove docker namespace from hostname
What this PR does:
Registers etcd's server proto in gogoproto codec
Which issue(s) this PR fixes:
Fixes #1213
Checklist
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]