-
Notifications
You must be signed in to change notification settings - Fork 302
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
Update the discovery service protocol for v3 discovery #583
Merged
+42
−52
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains 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
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.
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.
Not sure we can switch the curl to etcdctl as it is not obvious how developer would reimplement it. V2 discovery protocol was based on V2 api which was simple REST api that could be called via curl, showcasing how to implement it. Problem is that V3 api is using grpc which is binary protocol, so it's no longer obvious how we should explain it.
Could we maybe provide link to grpc proto definition and list which methods need to be implemented. It would be also great if we could provide examples by using client similar to curl however one native to grpc.
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 developers just need to follow the guide if they want to bootstrap a new cluster using v3 discovery. Probably we should add a simple example (as a new section) to help developers to understand it.
With regard to migrating from v2 discovery to v3 discovery, previously @ptabor has a comment . Both v2 & v3 discovery are only useful at the very first bootstrapping. Once etcd members have local data, they do not depend on the discovery service any more. So it doesn't make much sense to migrate the previous v2 discovery records?
We don't know the real data on how many users deploy the v2 discovery in their production environment. Most likely the use cases are very few. So users can manually perform the migration if they want.
What do you think?
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.
My understanding of this documentation is not for developer to learn it, but to be able to implement their service serving discover without deploying full etcd. For example https://github.com/etcd-io/discoveryserver. I would expect that this should be cough when we were migrating tests from V2 to V3, but it didn't.
I think so.
I think we could look into metrics of https://github.com/etcd-io/discoveryserver which is the public etcd service discovery maintained by etcd project.
Feels like we should also consider implementing V3 discovery in https://github.com/etcd-io/discoveryserver. However I would be worried to change and deploy a project that haven't seen much updates for last couple of years.
TODO:
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.
Not really per my understanding. The discovery.etcd.io or discoveryserver is just a router, which routes the client requests to backend etcd cluster. Please note that users do not necessarily to use the public discovery API, they can just deploy an etcd cluster as the discovery service themselves.
This is a good point. But just as I mentioned above, users do not necessarily use the public discovery API. But anyway, we can take a look at the metrics firstly. Do you or @ptabor know the info on the backend etcd server supporting the public discovery service?
I don't think we should implement a public discovery service for V3 discovery. But it's open to discussion.
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 read some discoveryserver documentation, you are right it's just a router to etcd server underneath. Question should we also implement it to support v3 discovery to avoid removing ability for users to use public discovery service in v3.6?
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.
A couple of comments:
This PR should be good.
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.
cc @ptabor