-
Notifications
You must be signed in to change notification settings - Fork 3.9k
core, grpclb: change policy selection strategy for Grpclb policy (take one: eliminate special logic for deciding grpclb policy in core) #6637
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
Merged
voidzcy
merged 8 commits into
grpc:master
from
voidzcy:impl/grpclb_selection_change_take_one
Jan 31, 2020
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…olution result. Change AutoConfiguredLoadBalancerFactory to choose LB policy purely based on lb policy config in service config. GrpclbLoadBalancer populates balancer address from attributes instead of from addresses.
4e33ec5
to
c090767
Compare
…election_change_take_one
c090767
to
da29532
Compare
creamsoup
reviewed
Jan 28, 2020
core/src/main/java/io/grpc/internal/AutoConfiguredLoadBalancerFactory.java
Show resolved
Hide resolved
…f no usable addresses.
…n balancer addresses, this mirrors the real behavior of DnsNameResolver.
a9d6a40
to
b428ae3
Compare
b428ae3
to
5adefa9
Compare
creamsoup
approved these changes
Jan 30, 2020
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, can you wait couple days. it would be nice to verify "service config error handling" without this change.
8f38f42
to
ab974ef
Compare
ab974ef
to
08cb3d8
Compare
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In this take:
DnsNameResolver
returns balancer addresses as aGrpcAttributes.ATTR_LB_ADDRS
attribute inResolutionResult
.AutoConfiguredLoadBalancerFactory
decides LB policy solely based on parsed service config without looking at resolved addresses. Behavior changes:pick_first
, even if there exist balancer addresses.grpclb
specified but not available and no other specified policies available, it will fail without fallback toround_robin
.GrpclbLoadBalancer
populates balancer addresses fromResolvedAddresses
's attribute (GrpclbConstants.ATTR_LB_ADDRS
) instead of sieving from addresses.Next step after this change is to separate logic for querying SRV record in
DnsNameResolver
to agrpclb
specific resolver.