Skip to content

Deprecate usage of gresolver.Address.Metadata #19706

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

Open
siyuanfoundation opened this issue Apr 3, 2025 · 9 comments · May be fixed by #19785
Open

Deprecate usage of gresolver.Address.Metadata #19706

siyuanfoundation opened this issue Apr 3, 2025 · 9 comments · May be fixed by #19785

Comments

@siyuanfoundation
Copy link
Contributor

What would you like to be added?

Deprecate usage of gresolver.Address.Metadata in client/v3/naming/resolver/resolver.go, and add Attributes field.

Why is this needed?

gresolver.Address.Metadata in grpc is deprecated, and should use Attributes instead.
https://github.com/grpc/grpc-go/blob/51d6a43ec59753d42ccd02bb12d2e9e40c164f0f/resolver/resolver.go#L117-L121

@amosehiguese
Copy link

Hi @siyuanfoundation, I'd like to work on implementing this feature. I'll follow the gRPC resolver API changes to deprecate the Metadata usage in favor of Attributes.

@amosehiguese
Copy link

Hi @siyuanfoundation

I just sent in a pr with the implementation in place.

@siyuanfoundation
Copy link
Contributor Author

/assign @amosehiguese

Thank you!

@siyuanfoundation
Copy link
Contributor Author

We should also have use the supported fields in tests like #19702

@amosehiguese
Copy link

amosehiguese commented Apr 4, 2025

Yeah, I noticed the tests still uses the metadata field, so my first attempt was to introduce the new field and make it backward compatible.

However, now that you've mentioned the Metadata field should remain in the Endpoint, I will revert back to that only and handle the conversion process underneath. If this is fine, please signify.

Lastly, I will love to mention that creating a new Attribute with the attributes.New(k, v any) to be used in the gresolver.Address Attribute field, takes in a key and a value as opposed to a single "any" argument(i.e the metadata passed)

Would you say it's okay to use the up.Key for the key and the up.Endpoint.Metadata for the value?

@ivanvc ivanvc removed the help wanted label Apr 5, 2025
amosehiguese added a commit to amosehiguese/etcd that referenced this issue Apr 9, 2025
This implementation adds the new Addributes field to resolver to better
align with gRPC's API as highlighted in issue etcd-io#19706

Signed-off-by: amosehiguese <[email protected]>
@ahrtr
Copy link
Member

ahrtr commented Apr 21, 2025

link to #15145

Let's follow the same pattern as grpc-go: add Attributes *attributes.Attributes into Endpoint, and deprecate Metadata

We might also want to backport the fix to release-3.6, depending on the complexity of the change.

cc @dfawley please chime in in case we miss anything, thx

@amosehiguese
Copy link

amosehiguese commented Apr 23, 2025

Let's follow the same pattern as grpc-go: add Attributes *attributes.Attributes into Endpoint, and deprecate Metadata

@ahrtr I had a PR open addressing that.
Do you want me reopen the pr?

Edited: I just realised you've aligned the resolver to use Endpoint.Addresses as opposed to the resolver.Address

@ahrtr
Copy link
Member

ahrtr commented Apr 23, 2025

Do you want me reopen the pr?

Yes, I think so. Please feel free to open the PR.

@amosehiguese
Copy link

Yes, I think so. Please feel free to open the PR.

I will be sending a fresh pr to align the API accordingly in a jiffy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

4 participants