xds: Hold parsed service config in CdsUpdate#12828
Open
ejona86 wants to merge 1 commit into
Open
Conversation
Member
Author
|
Whoa. Apparently I was on a really old commit when I started this. I don't really follow, but I'll need to rebase. Closing for the moment; I'll reopen when things are fixed up. |
This avoids re-parsing the config within CdsLB, as the providers could have changed and the config may no longer be valid. Many usages of ServiceConfigUtil.unwrapLoadBalancingConfig() were replaced with public API, which should be less brittle to internal changes. Similarly, config.equals() was added for LBs least_request, ring_hash, wrr to use more public APIs in testing. But I've gone out of my way to avoid using equals for XdsClient change detection, by preserving the original "JSON" config. This fixes a bug in WRR config parsing which prevented it from parsing errorUtilizationPenalty as it assumed it would be a Float, not a Double like our parser actually generates and our API requires. JsonUtil.getNumberAsFloat() was added specifically for WRR and has never worked as JSON Numbers will always be Doubles. In XdsClusterResource.CdsUpdate, the LB-specific fields like minRingSize were already not used at all, so this commit deletes them as it seems a relevant cleanup. Fixes grpc#12733
ejona86
force-pushed
the
xds-update-holds-parsed-config
branch
from
3effbb4 to
bb91229
Compare
Member
Author
|
Rebased on a recent commit now, instead of something from a year ago. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This avoids re-parsing the config within CdsLB, as the providers could have changed and the config may no longer be valid.
Many usages of ServiceConfigUtil.unwrapLoadBalancingConfig() were replaced with public API, which should be less brittle to internal changes. Similarly, config.equals() was added for LBs least_request, ring_hash, wrr to use more public APIs in testing. But I've gone out of my way to avoid using equals for XdsClient change detection, by preserving the original "JSON" config.
This fixes a bug in WRR config parsing which prevented it from parsing errorUtilizationPenalty as it assumed it would be a Float, not a Double like our parser actually generates and our API requires. JsonUtil.getNumberAsFloat() was added specifically for WRR and has never worked as JSON Numbers will always be Doubles.
In XdsClusterResource.CdsUpdate, the LB-specific fields like minRingSize were already not used at all, so this commit deletes them as it seems a relevant cleanup.
Fixes #12733
CC @lepistone