Skip to content

Conversation

arjan-bal
Copy link
Contributor

@arjan-bal arjan-bal commented Apr 4, 2025

Addresses: #8225

This PR makes the new pickfirst ignore the deprecated Metadata field. This is required since pickfirst is currently using the Medata field to lookup addresses in an addressList and not using the Metadata while looking up addresses from the AddressMap. This change makes the behaviour of both the data structures consistent.

RELEASE NOTES:

  • balancer/pickfirstleaf: Ignore the deprecated Address.Metadata field.

@arjan-bal arjan-bal changed the title balancer/pickfirstleaf: Handle metadata pf balancer/pickfirstleaf: Handle Addresses with Metadata Apr 4, 2025
Copy link

codecov bot commented Apr 4, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.04%. Comparing base (ce35fd4) to head (c290057).
Report is 13 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8227      +/-   ##
==========================================
- Coverage   82.16%   82.04%   -0.12%     
==========================================
  Files         410      412       +2     
  Lines       40248    40487     +239     
==========================================
+ Hits        33068    33219     +151     
- Misses       5830     5891      +61     
- Partials     1350     1377      +27     
Files with missing lines Coverage Δ
balancer/pickfirst/pickfirstleaf/pickfirstleaf.go 86.57% <100.00%> (-0.03%) ⬇️

... and 57 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@arjan-bal arjan-bal added this to the 1.72 Release milestone Apr 4, 2025
@arjan-bal arjan-bal requested review from dfawley and easwars and removed request for dfawley April 4, 2025 07:56
@arjan-bal arjan-bal force-pushed the handle-metadata-pf branch from b5b2dbc to c8fb55c Compare April 4, 2025 08:03
@arjan-bal arjan-bal changed the title balancer/pickfirstleaf: Handle Addresses with Metadata balancer/pickfirstleaf: Fix handling of Addresses with Metadata Apr 4, 2025
@easwars
Copy link
Contributor

easwars commented Apr 7, 2025

Do we still want to do this?

The most recent comment on etcd-io/etcd#19700 (comment) says that etcd itself is not affected by this and that they are going to prioritize the long term fix of not using the Metadata field over any short term fixes.

But I also see that your note saying pickfirstleaf is affected and that it does not work with Addresses that contain the Metadata field. Can we say that the new pick_first policy will not support the Metadata field since it has now been marked as deprecated for over 5 years and thereby simplify our lives?

@easwars easwars assigned arjan-bal and unassigned easwars and dfawley Apr 7, 2025
@arjan-bal
Copy link
Contributor Author

arjan-bal commented Apr 9, 2025

Waiting to discuss this with @dfawley. If we think is fine not to support Metadata to differentiate addresses in the new pickfirst, we would need to change the following condition. Having a test for the behaviour would be good.

func equalAddressIgnoringBalAttributes(a, b *resolver.Address) bool {
return a.Addr == b.Addr && a.ServerName == b.ServerName &&
a.Attributes.Equal(b.Attributes) &&
a.Metadata == b.Metadata
}

@ahrtr
Copy link

ahrtr commented Apr 14, 2025

The most recent comment on etcd-io/etcd#19700 (comment) says that etcd itself

etcd isn't affected, but etcd's users are affected. I think we still need the fix regardless the long-term plan (which is not yet finalized).

@arjan-bal arjan-bal changed the title balancer/pickfirstleaf: Fix handling of Addresses with Metadata balancer/pickfirstleaf: Avoid reading Address.Metadata Apr 15, 2025
@arjan-bal arjan-bal assigned easwars and dfawley and unassigned arjan-bal Apr 15, 2025
@arjan-bal arjan-bal assigned arjan-bal and unassigned easwars Apr 16, 2025
@arjan-bal arjan-bal merged commit d36887b into grpc:master Apr 16, 2025
31 of 33 checks passed
@arjan-bal arjan-bal deleted the handle-metadata-pf branch April 16, 2025 16:24
arjan-bal added a commit to arjan-bal/grpc-go that referenced this pull request Apr 16, 2025
janardhanvissa pushed a commit to janardhanvissa/grpc-go that referenced this pull request Apr 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants