Skip to content
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

balancer: stop using address.Metadata, use address.Attributes instead #3563

Open
menghanl opened this issue Apr 23, 2020 · 8 comments
Open
Labels
Area: Resolvers/Balancers Includes LB policy & NR APIs, resolver/balancer/picker wrappers, LB policy impls and utilities. P2 Type: Internal Cleanup Refactors, etc

Comments

@menghanl
Copy link
Contributor

address.Metadata is deprecated and replaced by address.Attributes.

The current usage of address.Metadata in gRPC includes:

  • grpclb, to pass rpc Metadata
  • weighted round robin, to pass weights

This issue is to track removing of these usages, and use address.Attributes instead.

@menghanl
Copy link
Contributor Author

cc @alazarev
FYI the way to pass address weights in weighted-round-robin is changing. The weights will be in attributes, instead of metadata, see the change here
The plan is to keep both for one release (v1.30.0), and to remove setting address.Metadata later (in v1.31.0)

@menghanl menghanl added the P2 label Apr 23, 2020
@menghanl menghanl changed the title balancer: stop using address.Metadata, use address.Attributes balancer: stop using address.Metadata, use address.Attributes instead Apr 23, 2020
@jhump
Copy link
Member

jhump commented May 8, 2020

FYI, the new formulation, a pointer *attributes.Attributes, means that the base balancer will now consider every address a "new address", and recycle every single connection, if the two addresses don't have the same pointer address for attributes. This seems onerous on resolver impls, to have to track an "intern"ed set of attributes for all permutations of attribute values, just so the pointer values can be stable and not cause tons of churn in connections.

This is because it uses resolver.Address as a map key for uniqueness, so an otherwise identical address but with different attributes pointer will be a distinct key.

@dfawley
Copy link
Member

dfawley commented May 8, 2020

Thanks for pointing this out, @jhump. We had discussed this awhile back, and knew it would be an issue, but I think we must have forgotten about it when we got around to implementing things. We will need to put together a solution for this ASAP as the use of Attributes is expected to expand.

@jhump
Copy link
Member

jhump commented May 9, 2020

@dfawley, thanks for the reply. I filed #3611 for this.

@easwars easwars self-assigned this May 4, 2021
@easwars easwars added the fixit label May 4, 2021
@easwars
Copy link
Contributor

easwars commented May 4, 2021

The issue of making attributes.Attributes comparable is tracked under #3611.

We have fixed the WRR implementation to pass weights through attributes. But it still also passes the same through metadata. This can be removed based on [comment](https://github.com/grpc/grpc-go/issues/3563#issuecomment-618680599) which says that we can remove it in v1.31.0`.

The other item is to see if we want any changes in grpclb as this is mentioned in comment

@easwars easwars removed their assignment May 4, 2021
@zasweq
Copy link
Contributor

zasweq commented May 16, 2022

@easwars I can't seem to find any more instances of address.Metadata in the codebase. Should we close this?

@easwars
Copy link
Contributor

easwars commented May 16, 2022

I do see usages in the ringhash balancer.

weightSum += a.Metadata.(uint32)

@menghanl : Was this intentional? Can we use the attributes field here?

Also, we should probably investigate if we can completely get rid of the field from resolver.Address and delete existing references here and here. @dfawley

@dfawley
Copy link
Member

dfawley commented Nov 28, 2022

I believe the only place left that we use this internally is a read in the transport to provide the legacy behavior of setting outgoing headers based on it. We never set it. It should be okay to remove now, though it would be a breaking (deprecated + experimental) API change.

@arjan-bal arjan-bal added the Area: Resolvers/Balancers Includes LB policy & NR APIs, resolver/balancer/picker wrappers, LB policy impls and utilities. label Sep 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Resolvers/Balancers Includes LB policy & NR APIs, resolver/balancer/picker wrappers, LB policy impls and utilities. P2 Type: Internal Cleanup Refactors, etc
Projects
None yet
Development

No branches or pull requests

6 participants