xds: change cdsbalancer to use dependency manager and remove clusterresolver#8777
xds: change cdsbalancer to use dependency manager and remove clusterresolver#8777eshitachandwani wants to merge 16 commits intogrpc:masterfrom
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #8777 +/- ##
==========================================
- Coverage 83.42% 83.22% -0.20%
==========================================
Files 418 411 -7
Lines 32897 32529 -368
==========================================
- Hits 27443 27072 -371
- Misses 4069 4074 +5
+ Partials 1385 1383 -2
🚀 New features to boost your workflow:
|
|
The test is still failing. |
| // DependencyManagerKey is the type used as the key to store DependencyManager | ||
| // in the Attributes field of resolver.states. | ||
| type DependencyManagerKey struct{} |
There was a problem hiding this comment.
The key type should be private. Other packages must use the public getters and setters.
| // GetRefCount returns the reference count for a particluar cluster. | ||
| func (c *ClusterRef) GetRefCount() int32 { | ||
| return atomic.LoadInt32(&c.refCount) | ||
| } |
There was a problem hiding this comment.
Methods that simply return the value of an atomic are prone to race conditions and difficult to use correctly. A caller might read GetRefCount and take action, but the value could change before that action completes. I'm reviewing the surrounding code to see if this race actually manifests here.
|
|
||
| func (pickfirstBuilder) ParseConfig(js json.RawMessage) (serviceconfig.LoadBalancingConfig, error) { | ||
| var cfg pfConfig | ||
| var cfg PfConfig |
There was a problem hiding this comment.
We should not need to export the config here. In the test, we cast use the pickfirst builder into a config parser and make it return the struct for a particular json. Example:
Exporting the config would allow external users to refer to the symbol and removing it would be a breaking change.
| m.dnsSerializerCancel() | ||
| for name, dnsResolver := range m.dnsResolvers { | ||
| dnsResolver.stop() | ||
| if dnsResolver.extras.dnsR != nil { |
There was a problem hiding this comment.
When can dnsResolver.extras be nil? If it is nil, why is it being stored in the map?
| @@ -203,7 +202,9 @@ func (m *DependencyManager) Close() { | |||
| // try to grab the dependency manager lock, which is already held here. | |||
| m.dnsSerializerCancel() | |||
| for name, dnsResolver := range m.dnsResolvers { | |||
There was a problem hiding this comment.
The variable naming seems confusing here. The field named dnsResolver seems to the resource state of a logical DNS cluster. Should the field name be updated?
|
Just realized that this PR has been split as requested. I'm continuing the review on the smaller PRs. Please address these comments in the smaller PRs and then close this one. |
3 participants
Part of A74 changes. This PR changes the
cdsbalancerto use the XDSConfig for resources and remove the cluster watcher from cds balancer.Also removes the EDS/DNS watchers from the balancers and remove cluster resolver policy completely.
Also moves the e2e tests in the
clusterresolverpackage tocdsbalancerpackage.The PR also exports the pick first LB config to be used in tests, will change it back when #8733 is merged and we can configure round robin as child policy for DNS clusters.
RELEASE NOTES: