-
Notifications
You must be signed in to change notification settings - Fork 9.8k
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
Replacement API for client/v3/naming package to be compatible with new GRPC1.30+ resolver API. #12614
Replacement API for client/v3/naming package to be compatible with new GRPC1.30+ resolver API. #12614
Conversation
da6fdfd
to
4a5fbbf
Compare
for me it looks the way it must to be |
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.
+1 for the package renaming here. This is mostly moving some of the naming resolution code under client/v3
-- do I understand right?
Can you add more details regarding my comments in the PR?
@@ -0,0 +1,37 @@ | |||
package internal |
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.
Where do we use this package?
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.
Inside implementation of the endpoint.Manager. It describes how we serialize and deserialize the endpoints as JSON, that we persist in etcd.
As client/v3 is being is linked into many customer's application, it's important to distinguish public API surface from implementation details. We could keep it lower-case as well, but I wanted in this PR stress its private.
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 understand that, but client/v3/naming/endpoints/endpoints_impl.go
importing this looks like placeholders and not implemented yet?
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.
Yes. I hope for contributors to implement this within a month. If no one will contribute, I will do the implementation itself.
The endpoints definition is fixed in stone so its why I'm submitting it.
b9b43de
to
11831ce
Compare
@gyuho : PTAL |
85f12bd
to
4503591
Compare
@gyuho I changed the PR such that its commitable (I left original naming/grpc.go and introduced the new interfaces side by side, and "stashed" the grpcproxy migration&cleanup ). @scDisorder @dhermes @skyao FYI. |
glad to see that! seems like there is not so much to handle ahead i'll be literally in touch, when PR would be merged, you can notify me |
@gyuho Are you OK with me merging this ? |
@@ -0,0 +1,121 @@ | |||
package endpoints | |||
|
|||
// TODO: The API is not yet implemented. |
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.
Can we move this as backlogged GitHub issue? Or put these into internal
package? Once we have this public, it's hard to remove. Seems like this has not been implemented?
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.
Added the issue:
#12652
I assume this will get implemented as part of 3.5.0 (I will contribute if none else).
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.
@ptabor Thanks!
This has not been implemented "in" etcd, but I understand this is mainly to unblock gRPC dependency upgrades, so we should be ok to leave it not implemented. Or if we aren't going to have example endpoint manager implementation, can we just remove? |
4503591
to
dd79c6c
Compare
This is not yet implementation, just API and tests to be filled with implementation in next CLs, tracked by: etcd-io#12652 We propose here 3 packages: - clientv3/naming/endpoints -> That is abstraction layer over etcd that allows to write, read & watch Endpoints information. It's independent from GRPC API. It hides the storage details. - clientv3/naming/endpoints/internal -> That contains the grpc's compatible Update class to preserve the internal JSON mashalling format. - clientv3/naming/resolver -> That implements the GRPC resolver API, such that etcd can be used for connection.Dial in grpc. Please see the grpc_naming.md document changes & grpcproxy/cluster.go new integration, to see how the new abstractions work.
dd79c6c
to
5d7c1db
Compare
This is not yet implementation, just API definitions.
Please review whether the API is good / can be improved. The goal is to reach agreement about
desired layout of the naming/resolver packages in etcd-3.5+, before investement in the implementation of the API.
We propose here 3 packages:
clientv3/naming/endpoints ->
That is abstraction layer over etcd that allows to write, read &
watch Endpoints information. It's independent from GRPC API. It hides
the storage details.
clientv3/naming/endpoints/internal ->
That contains the grpc's compatible Update class to preserve the
internal JSON mashalling format.
clientv3/naming/resolver ->
That implements the GRPC resolver API, such that etcd can be
used for connection.Dial in grpc.
Please see the [grpc_naming.md](TLDR: Please take a look at the proposed modified 3.5 grpc_naming user guide document changes & grpcproxy/cluster.go new integration, to see how the new abstractions work.
Note: I'm trying to reach agreement on the vision. I would appreciate (when/if accepted) contributions covering the implementation aspect.
@skyao @mcluseau @scDisorder @xiang90