core/grpclb: resolve TXT records in DNS name resolver and include balancer addresses #3852
Conversation
|
|
||
| @VisibleForTesting | ||
| static boolean enableJndi = false; | ||
| static boolean enableJndi = true; |
There was a problem hiding this comment.
We should keep this off until it is stable.
There was a problem hiding this comment.
Put it behind a flag so that it can be tested before release.
|
For C-core, we have some tests that use a DNS server. You can talk to @apolcyn about how this was set up. |
|
@zhangkun83 ping |
zhangkun83
left a comment
zhangkun83
left a comment
There was a problem hiding this comment.
Sorry for the delay. Overall LGTM, except for some minor comments.
Please prefix the PR title with "core/grpclb:" as we always do. The title is also a bit misleading, because we are not providing generic SRV results, but rather balancer addresses carried in SRV results. This should be made clear.
| ResolutionResults( | ||
| List<InetAddress> addresses, | ||
| List<String> txtRecords, | ||
| List<EquivalentAddressGroup> srvRecords) { |
There was a problem hiding this comment.
These are actually balancer addresses. They are not intended to be generic SRV records. Please name the variables (including those in CompositeResolver) accordingly.
| Attributes.Builder attrs = Attributes.newBuilder(); | ||
| if (!resolvedInetAddrs.txtRecords.isEmpty()) { | ||
| attrs.set( | ||
| GrpcAttributes.NAME_RESOLVER_ATTR_TXT, |
There was a problem hiding this comment.
TXT record is DNS-specific. Let's add "DNS" to the name of this attribute.
carl-mastrangelo
changed the title
carl-mastrangelo
changed the title
4 participants
This is part one of handling service config, and also enabled grpclb to work. I manually tested this with some complex DNS data, but I dont have many unit tests because we dont have a dummy DNS server.
Handling or the SRV records logs failures and tries to keep going.
This can be enabled by setting
io.grpc.internal.DnsNameResolverProvider.enable_jndito true. This feature enables https://github.com/grpc/proposal/blob/master/A5-grpclb-in-dns.md