Skip to content

xds: use separate LB configs for EDS policy running with different code paths #6895

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

Conversation

voidzcy
Copy link
Contributor

@voidzcy voidzcy commented Apr 2, 2020

In the latest design, the LB config for EDS policy started to diverge for the full xDS flow and for EDS only flow (internal. legacy?).

This changes separates the usage of LB configs for these two code paths:

For full xDS flow (received from CDS LB policy):

class EdsConfig {
    final String clusterName;  // required
    final String edsServiceName;  // optional
    final String lrsServerName;  // optional
    final PolicySelection endpointPickingPolicy;  // required
}

For EDS only flow (received from remote resolver):

class XdsConfig {
    final String edsServiceName;  // optional
    final String lrsServerName;  // optional
    final PolicySelection childPolicy;  // required, parser defaults to round_robin
    final PolicySelection fallbackPolicy;  // required, parser defaults to round_robin
}

Since the EDS only flow (XdsLoadBalancer) also delegates load balancing logic to EdsLoadBalancer, it converts XdsConfig to EdsConfig by setting clusterName field to target authority.


This PR only cleans up LB config(s) (types) for EDS policy, no behavior-wise change.

This PR replaces #6887, which tries to do more cleanup (e.g., refactor FallbackLb, move logic of creating XdsClient object into XdsLoadBalancer) but failed to do so without rewriting the whole set of tests.

@voidzcy voidzcy requested a review from dapengzhang0 April 2, 2020 21:20
@voidzcy voidzcy changed the title xds: use separate LB config for EDS policy running with different code paths xds: use separate LB configs for EDS policy running with different code paths Apr 2, 2020
…separate_lb_config_for_full_flow_and_eds_only
@dapengzhang0 dapengzhang0 self-assigned this Apr 14, 2020
"Received EDS lb config: cluster={0}, child_policy={1}, "
+ "eds_service_name={2}, report_load={3}",
newEdsConfig.clusterName,
newEdsConfig.endpointPickingPolicy != null
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

endpointPickingPolicy is not null

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@@ -91,7 +96,8 @@ public void onAllDrop() {
Helper helper,
PrimaryLbFactory primaryLbFactory,
LoadBalancer.Factory fallbackLbFactory) {
this.helper = helper;
this.helper = checkNotNull(helper, "helper");
authority = checkNotNull(helper.getAuthority(), "authority");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: This field is only used in one place, and can be inlined with helper.getAuthority() there. No need to check nullness of helper's authority.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@voidzcy voidzcy merged commit 2478912 into grpc:master Apr 15, 2020
dfawley pushed a commit to dfawley/grpc-java that referenced this pull request Jan 15, 2021
…de paths (grpc#6895)

The LB configs used for EDS policy diverges for the full xDS flow (generated by CDS policy) and EDS-only flow (received in service config). This change creates a separate config (EdsConfig) for the actual EDS LB policy. CDS policy generates EdsConfig directly and the wrapper policy (i.e., XdsLoadBalancer) converts received XdsConfig to EdsConfig for EDS-only flow.
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 15, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants