resolver: Add resolver.EndpointMap.All() to iterate over keys and values#8867
Conversation
It's nicer than iterating over the keys and looking up their values. RELEASE NOTES: N/A
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #8867 +/- ##
==========================================
+ Coverage 82.42% 83.04% +0.62%
==========================================
Files 414 414
Lines 32750 32818 +68
==========================================
+ Hits 26994 27255 +261
- Misses 4096 4104 +8
+ Partials 1660 1459 -201
🚀 New features to boost your workflow:
|
arjan-bal
left a comment
There was a problem hiding this comment.
Hi @Jille, thanks for the contribution. In addition to being more ergonomic, the iterator approach is a nice optimization as it avoids allocating a slice for the return values.
Could you also update existing call sites of Keys() and Values() within the codebase to use the new iterator methods where it makes sense? This would help validate the new API and provide better coverage.
|
Thanks for the review @arjan-bal :) |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new All() method to AddressMapV2 and EndpointMap for iterating over key-value pairs, which is a great improvement for code clarity and efficiency. The refactoring to adopt this new method is mostly well-executed. However, I've identified a few instances where the new All() method isn't fully utilized, leading to redundant Get() calls that this PR aims to eliminate. My suggestions below address these points to better align the changes with the PR's goal.
arjan-bal
left a comment
There was a problem hiding this comment.
LGTM! Adding a second reviewer. Please check the minor suggestions from Gemini.
ef20789 to
325e666
Compare
|
I'd swear I checked whether I saw that pattern. Maybe I only checked for EndpointMap and not AddressMap. Applying Gemini's changes broke the indentation, so I overwrote that. Also, hey Easwar :) |
Yes. Long time. Good to see you here. |
|
@arjan-bal |
|
Yes, we discussed this in the gRPC Go maintainers group and decided to deprecate the |
Moved the comment from EndpointMap.Keys() to EndpointMap.All()
|
np. Done. |
|
Thanks for your contribution, @Jille ! |
…ues (grpc#8867) It's nicer than iterating over the keys and looking up their values. RELEASE NOTES: * resolver: Add `All()` methods to `AddressMap` and `EndpointMap` that return iterators for efficient key-value pair traversal.
…ues (grpc#8867) It's nicer than iterating over the keys and looking up their values. RELEASE NOTES: * resolver: Add `All()` methods to `AddressMap` and `EndpointMap` that return iterators for efficient key-value pair traversal.
It's nicer than iterating over the keys and looking up their values.
RELEASE NOTES:
All()methods toAddressMapandEndpointMapthat return iterators for efficient key-value pair traversal.