Skip to content

Conversation

@voidzcy
Copy link
Contributor

@voidzcy voidzcy commented Jun 4, 2021

Envoy uses default parameters for ring hash LB config (min_ring_size = 1024, max_ring_size = 8M). grpc/proposal#239 is updated to use the same default as Envoy.

The change follows the way that C-core does this:

  • gRPC's ring_hash LB config parser sets default values if the raw config does not specify min_ring_size or max_ring_size.
    • Note, for us we need to do this in both ring_hash LB config parser and the CDS LB policy that instantiates the RingHashConfig directly.

/cc @apolcyn

@voidzcy voidzcy marked this pull request as draft June 4, 2021 19:55
@voidzcy voidzcy changed the title xds: add some validation for ring_hash_lb_config xds: use defaults for unspecified ring_hash_lb_config values Jun 4, 2021
@voidzcy voidzcy marked this pull request as ready for review June 4, 2021 21:59
@voidzcy voidzcy requested a review from dapengzhang0 June 7, 2021 17:11
long minRingSize = lbConfig.getMinimumRingSize().getValue();
long maxRingSize = lbConfig.getMaximumRingSize().getValue();
if (lbConfig.getHashFunction() != RingHashLbConfig.HashFunction.XX_HASH
|| minRingSize > maxRingSize) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Your change in CdsLoadBalancer2.java seems to allow both minRingSize and maxRingSize to be zero, but here it is not allowed?

@voidzcy
Copy link
Contributor Author

voidzcy commented Jun 7, 2021

Updated how default values are applied.

We want the XdsClient to do as much validation as possible and NACK invalid configurations earlier better than later. We need to know the default values in order to implement the full set of validations. For example, {min_ring_size = 100} should be considered as a valid config, as max_ring_size will be default to 8M. The validation has to take default values into consideration.

So we implement setting default values in XdsClient. However, there is another codepath we need to consider, which is the LB config parser. Currently, this part is dead code (and most LB policies except ring_hash do not implement it). It exists because ideally we would support pluggable LB policies with LB configs given by any name resolution mechanism.

Therefore, we inevitably introduce some duplicated logic, just because the architecture is intended to be pluggable for each component. Same thing is happening in C-core: XdsClient parsing ring_hash LB config and the ring_hash LB policy's own config parser parses JSON config.

I think we should do the same for circuit breaker's default (will send a PR for that after this one is resolved).

Copy link
Contributor

@dapengzhang0 dapengzhang0 left a comment

Choose a reason for hiding this comment

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

LGTM

@voidzcy voidzcy merged commit fa4b980 into grpc:master Jun 7, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 6, 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