xds: support BalancerV2 API #2781
Conversation
balancer/xds/xds.go
Outdated
| x.fallbackLB = builder.Build(x.cc, x.buildOpts) | ||
|
|
||
| if x.fallbackInitData != nil { | ||
| // TODO: uncomment when HandleBalancerConfig API is merged. |
There was a problem hiding this comment.
I would like to clean up this at later time. Essentially, it's essentially make fallback balancer which implements V2 API gets both service config and addresses.
There was a problem hiding this comment.
I have a feeling that this won't be as trivial as it sounds. It may change the way you handle resolver updates in this PR.
balancer/xds/xds.go
Outdated
| x.startFallBackBalancer(u) | ||
|
|
||
| if u.addrUpdate != nil && x.fallbackLB != nil { | ||
| x.fallbackLB.HandleResolvedAddrs(u.addrUpdate.addrs, u.addrUpdate.err) |
There was a problem hiding this comment.
If this fallback balancer is new, it will handle the same addresses twice.
balancer/xds/xds.go
Outdated
| if u.addrUpdate != nil { | ||
| // addresses have been updated. | ||
| // in case of x.config == nil where it returns early, we set the fallbackInitData here. | ||
| x.fallbackInitData = u.addrUpdate |
There was a problem hiding this comment.
Why do you still need fallbackInitData?
If fallback balancer is updated, the new addresses are also sent along with the update.
You shouldn't need to keep the old addresses for a new fallback balancer.
There was a problem hiding this comment.
It's possible that fallback balancer is changed at later time (and the addresses are not updated in the same update), so we still need to initialize with the old addresses.
There was a problem hiding this comment.
Actually, you need this field when switching to fallback.
But this update here shouldn't be necessary. It also causes double address update for a new fallback balancer.
There was a problem hiding this comment.
It's here because of the early return. Let me think a bit more if we can just move this line inside the early return condition.
balancer/xds/xds.go
Outdated
| var update interface{} | ||
| // if service config does not change for this update, we only send address update. | ||
| if x.lastResolverUpdate != nil && x.lastResolverUpdate.ServiceConfig == s.ServiceConfig { | ||
| update = &resolverUpdate{ |
There was a problem hiding this comment.
Should this function just sent a &resolverUpdate{...} to the executor goroutine, and do all the checks and parsing there?
There was a problem hiding this comment.
Why we want to do this?
There was a problem hiding this comment.
Some of the logic here and later in the goroutine is duplicate.
For example:
Each resolver update, the balancer does:
- if addresses are updated, do something for the addresses
- if config is updated, do something for the config
Checks like this are done twice.
First time here,
- if addresses are updated,
resolverUpdate.addrUpdateis set - if config is updated,
resolverUpdate.xdsconfigis set
Later in the goroutine
- if
resolverUpdate.addrUpdateis not nil, do something - if
resolverUpdate.xdsconfigis not nil, do something
The code is this way because the resolver update is still treated as two separate things. I think the code should be updated to actually treat them as one.
The update sent to the goroutine can be just
type resolverUpdate struct {
state resolver.State
}
3 participants
No description provided.