Replacement API for client/v3/naming package to be compatible with new GRPC1.30+ resolver API. #12614

ptabor · 2021-01-12T23:43:40Z

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

scDisorder · 2021-01-15T15:52:32Z

for me it looks the way it must to be
and it resolves an issue by the way of grpc documentation
lgtm

gyuho

+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?

client/v3/naming/endpoints/endpoints.go

client/v3/naming/doc.go

client/v3/naming/endpoints/endpoints.go

gyuho · 2021-01-17T06:24:48Z

client/v3/naming/endpoints/internal/update.go

@@ -0,0 +1,37 @@
+package internal


Where do we use this package?

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.

I understand that, but client/v3/naming/endpoints/endpoints_impl.go importing this looks like placeholders and not implemented yet?

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.

server/proxy/grpcproxy/cluster.go

client/v3/naming/grpc.go

ptabor · 2021-01-18T13:31:57Z

@gyuho : PTAL

ptabor · 2021-01-22T20:50:59Z

@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 ).
I propose merging this PR, to unblock other contributors interested in enabling grpc upgrade.

@scDisorder @dhermes @skyao FYI.

scDisorder · 2021-01-25T10:57:34Z

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

scDisorder · 2021-01-29T08:32:22Z

so how it is going? :)

@ptabor @gyuho

sorry if I do bother you :)

ptabor · 2021-01-29T22:42:30Z

@gyuho Are you OK with me merging this ?

gyuho · 2021-01-30T04:28:36Z

client/v3/naming/endpoints/endpoints_impl.go

@@ -0,0 +1,121 @@
+package endpoints
+
+// TODO: The API is not yet implemented.


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?

Added the issue:
#12652
I assume this will get implemented as part of 3.5.0 (I will contribute if none else).

@ptabor Thanks!

gyuho · 2021-01-30T04:34:45Z

endpoints.NewManager(c, "foo")

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?

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.

ptabor changed the title ~~POC: Migrate client/v3/naming package to be compatible with new GRPC resolver API.~~ API POC: Migrate client/v3/naming package to be compatible with new GRPC resolver API. Jan 12, 2021

This was referenced Jan 12, 2021

Prepar to upgrade grpc version: grpc naming packge is moved in v1.30.0 #12609

Closed

etcd not compatible with grpc v1.30.0 #12124

Closed

wenjiaswe requested a review from gyuho January 14, 2021 19:16

gyuho self-assigned this Jan 14, 2021

ptabor force-pushed the prepare-to-remove-grpc-naming-package branch from da6fdfd to 4a5fbbf Compare January 14, 2021 22:57

ptabor mentioned this pull request Jan 15, 2021

Prepar to upgrade grpc client version to v1.35.0 #12626

Closed

gyuho suggested changes Jan 17, 2021

View reviewed changes

ptabor force-pushed the prepare-to-remove-grpc-naming-package branch 2 times, most recently from b9b43de to 11831ce Compare January 18, 2021 13:28

dhermes mentioned this pull request Jan 20, 2021

Upgrade to google.golang.org/grpc@1.35.0 #12636

Closed

ptabor force-pushed the prepare-to-remove-grpc-naming-package branch 3 times, most recently from 85f12bd to 4503591 Compare January 22, 2021 19:59

ptabor changed the title ~~API POC: Migrate client/v3/naming package to be compatible with new GRPC resolver API.~~ Replacement API for client/v3/naming package to be compatible with new GRPC1.30+ resolver API. Jan 22, 2021

ptabor requested a review from gyuho January 22, 2021 20:51

gyuho reviewed Jan 30, 2021

View reviewed changes

ptabor mentioned this pull request Jan 30, 2021

grpc bump: Implement master/client/v3/naming new endpointManager & resolver #12652

Closed

ptabor force-pushed the prepare-to-remove-grpc-naming-package branch from 4503591 to dd79c6c Compare January 30, 2021 11:16

ptabor force-pushed the prepare-to-remove-grpc-naming-package branch from dd79c6c to 5d7c1db Compare January 30, 2021 11:32

ptabor merged commit cf2153d into etcd-io:master Jan 30, 2021

ptabor deleted the prepare-to-remove-grpc-naming-package branch January 30, 2021 11:51

This was referenced Oct 18, 2023

[3.4] Upgrade grpc-go to 1.29.1 and Backport "Introduce grpc-1.30+ compatible client/v3/naming API." #16795

Merged

[3.4] Backport clientv3 naming implementation #16800

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

ptabor commented Jan 12, 2021 •

edited

Loading

scDisorder commented Jan 15, 2021

gyuho left a comment

gyuho Jan 17, 2021

ptabor Jan 17, 2021 •

edited

Loading

gyuho Jan 30, 2021

ptabor Jan 30, 2021

ptabor commented Jan 18, 2021 •

edited

Loading

ptabor commented Jan 22, 2021

scDisorder commented Jan 25, 2021 •

edited

Loading

scDisorder commented Jan 29, 2021 •

edited

Loading

ptabor commented Jan 29, 2021

gyuho Jan 30, 2021

ptabor Jan 30, 2021

gyuho Jan 30, 2021

gyuho commented Jan 30, 2021

		@@ -0,0 +1,121 @@
		package endpoints

		// TODO: The API is not yet implemented.

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

ptabor commented Jan 12, 2021 • edited Loading

scDisorder commented Jan 15, 2021

gyuho left a comment

Choose a reason for hiding this comment

gyuho Jan 17, 2021

Choose a reason for hiding this comment

ptabor Jan 17, 2021 • edited Loading

Choose a reason for hiding this comment

gyuho Jan 30, 2021

Choose a reason for hiding this comment

ptabor Jan 30, 2021

Choose a reason for hiding this comment

ptabor commented Jan 18, 2021 • edited Loading

ptabor commented Jan 22, 2021

scDisorder commented Jan 25, 2021 • edited Loading

scDisorder commented Jan 29, 2021 • edited Loading

ptabor commented Jan 29, 2021

gyuho Jan 30, 2021

Choose a reason for hiding this comment

ptabor Jan 30, 2021

Choose a reason for hiding this comment

gyuho Jan 30, 2021

Choose a reason for hiding this comment

gyuho commented Jan 30, 2021

ptabor commented Jan 12, 2021 •

edited

Loading

ptabor Jan 17, 2021 •

edited

Loading

ptabor commented Jan 18, 2021 •

edited

Loading

scDisorder commented Jan 25, 2021 •

edited

Loading

scDisorder commented Jan 29, 2021 •

edited

Loading