Service Discovery support for etcd v3#663
Merged
peterbourgon merged 2 commits intogo-kit:masterfrom Feb 15, 2018
cabrel:master
Merged
Service Discovery support for etcd v3#663peterbourgon merged 2 commits intogo-kit:masterfrom cabrel:master
peterbourgon merged 2 commits intogo-kit:masterfrom
cabrel:master
Conversation
This adds a go-kit/kit/sd implementation for etcd/clientv3. All tests except the client_test were ported over. I have removed the client tests because they are more problematic with regards to etcd v3. The etcd maintainers switched to protobufs in v3 and placed those generated messages under internal/, which would cause issues trying to reference it (see https://golang.org/s/go14internal). This makes stubbing clientv3.KV more difficult without burying the implementation behind interfaces to remove the direct dependence on those internal packages. Features in use with v3: - Support for etcd/clientv3.Watcher - Support for etcd/clientv3.Lease (TTL) - Username/password support
Member
|
Very cool! I'll give this a review in the next day or two. Thanks! |
peterbourgon
requested changes
Feb 12, 2018
Member
peterbourgon
left a comment
peterbourgon
left a comment
There was a problem hiding this comment.
A few minor changes but this looks good! Thanks especially for the work on the tests!
sd/etcdv3/client.go
Outdated
| // ErrNoValue indicates a client method needs a value but receives none. | ||
| ErrNoValue = errors.New("no value provided") | ||
|
|
||
| ErrKeyNotFound = errors.New("requested key not found") |
| c.wcf() | ||
| } | ||
| return nil | ||
| } |
Member
There was a problem hiding this comment.
Does this function need to return an error?
| @@ -0,0 +1 @@ | |||
| package etcdv3 | |||
Member
There was a problem hiding this comment.
One or two sentences of doc comment would be appreciated!
Contributor
Author
There was a problem hiding this comment.
I copied and updated the doc comment from sd/etcd to sd/etcdv3 for consistency sake.
sd/etcdv3/registrar.go
Outdated
| // return | ||
| // } | ||
| // } | ||
| // } |
| func (tc *testClient) CancelWatch() {} | ||
|
|
||
| func (tc *testClient) WatchPrefix(prefix string, ch chan struct{}) { | ||
| } |
sd/etcdv3/registrar_test.go
Outdated
| {errors.New("regError"), "key=testKey value=testValue err=regError\n"}, | ||
| // test case: registration successful | ||
| {nil, "key=testKey value=testValue action=register\n"}, | ||
| } |
Member
There was a problem hiding this comment.
Can you define this in the test function rather than in the global scope?
sd/etcdv3/registrar_test.go
Outdated
| {errors.New("deregError"), "key=testKey value=testValue err=deregError\n"}, | ||
| // test case: deregistration successful | ||
| {nil, "key=testKey value=testValue action=deregister\n"}, | ||
| } |
Member
There was a problem hiding this comment.
Can you define this in the test function rather than in the global scope?
- moved global structs inside of test functions since they are not used anywhere else - moved unused Err variable - removed error return type from close() - added doc comment
peterbourgon
approved these changes
Feb 15, 2018
Member
|
Thank you very much! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This adds a go-kit/kit/sd implementation for etcd/clientv3, which was requested in #505
All tests except the client_test were ported over which I had to remove because they are more problematic with regards to etcd v3. The etcd maintainers switched to protobufs in v3 and placed those generated messages under internal/, which would cause issues trying to reference it (see https://golang.org/s/go14internal), this makes stubbing clientv3.KV more difficult without burying the
implementation behind interfaces to remove the direct dependence on those internal packages.
Features in use with v3: