Skip to content
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

clusterimpl: Switch Child LB to Graceful Switch #10006

Closed
zasweq opened this issue Apr 3, 2023 · 1 comment · Fixed by #10091
Closed

clusterimpl: Switch Child LB to Graceful Switch #10006

zasweq opened this issue Apr 3, 2023 · 1 comment · Fixed by #10091
Assignees
Labels
Milestone

Comments

@zasweq
Copy link
Contributor

zasweq commented Apr 3, 2023

Me Eric and Michael (who was guided by Mark) came to the conclusion that the Child LB of the cluster impl should be a Graceful Switch Balancer. This is needed to correctly support the scenario where the Child Type changes, and is currently a correctness issue.

Eric also mentioned writing a cross language interop test for this case.

@ejona86
Copy link
Member

ejona86 commented Apr 4, 2023

It seems the code only uses the child policy type the first time it is called. I the child type changes, this could easily cause a ClassCastException and cause the channel to panic.

if (cluster == null) {
cluster = config.cluster;
edsServiceName = config.edsServiceName;
childLbHelper = new ClusterImplLbHelper(
callCounterProvider.getOrCreate(config.cluster, config.edsServiceName),
config.lrsServerInfo);
childLb = config.childPolicy.getProvider().newLoadBalancer(childLbHelper);
// Assume load report server does not change throughout cluster lifetime.
if (config.lrsServerInfo != null) {
dropStats = xdsClient.addClusterDropStats(config.lrsServerInfo, cluster, edsServiceName);
}
}
childLbHelper.updateDropPolicies(config.dropCategories);
childLbHelper.updateMaxConcurrentRequests(config.maxConcurrentRequests);
childLbHelper.updateSslContextProviderSupplier(config.tlsContext);
childLb.handleResolvedAddresses(
resolvedAddresses.toBuilder()
.setAttributes(attributes)
.setLoadBalancingPolicyConfig(config.childPolicy.getConfig())
.build());

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants