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

clusterresolver: merge P(p)arseConfig functions #5462

Merged
merged 2 commits into from Jun 24, 2022

Conversation

easwars
Copy link
Contributor

@easwars easwars commented Jun 23, 2022

Fixes #5369

Apart from fixing the problem mentioned in #5369, this PR also performs the following cleanup:

  • remove const aliases for roundrobin.Name and ringhash.Name and use the actual consts themselves
  • address a TODO calling for the registration of a stub balancer to be removed once ring_hash is implemented

RELEASE NOTES: none

@easwars easwars requested a review from zasweq June 23, 2022 18:06
@easwars easwars added the Type: Internal Cleanup Refactors, etc label Jun 23, 2022
@easwars easwars added this to the 1.48 Release milestone Jun 23, 2022
xds/internal/balancer/clusterresolver/config.go Outdated Show resolved Hide resolved
b := balancer.Get(Name)
if b == nil {
t.Fatalf("LB policy %q not registered", Name)
}
cfgParser, ok := b.(balancer.ConfigParser)
if !ok {
t.Fatalf("LB policy %q does not support config parsing", Name)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think of this instead, parser := bb{};..... parser.ParseConfig() rather than this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know we do such things at a lot of places. But I feel that even though my version is a little more verbose, it is using the exported API, rather than relying on internals. More often that not, I feel that in tests, this approach serves better in the long run.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough. Doug left me that suggestion on my Outlier Detection PR mainly because I hadn't coded balancer implementation yet.

@@ -168,18 +168,13 @@ type LBConfig struct {
XDSLBPolicy *internalserviceconfig.BalancerConfig `json:"xdsLbPolicy,omitempty"`
}

const (
rrName = roundrobin.Name
rhName = ringhash.Name
Copy link
Contributor

Choose a reason for hiding this comment

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

Personally, I don't mind the alias since it saves verbosity, but I'm indifferent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ringhash.Name is not very long compared to rhName but it is way more descriptive.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough.

@zasweq zasweq assigned easwars and unassigned zasweq Jun 23, 2022
@easwars easwars assigned zasweq and unassigned easwars Jun 24, 2022
zasweq
zasweq approved these changes Jun 24, 2022
Copy link
Contributor

@zasweq zasweq left a comment

Choose a reason for hiding this comment

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

LGTM.

@@ -168,18 +168,13 @@ type LBConfig struct {
XDSLBPolicy *internalserviceconfig.BalancerConfig `json:"xdsLbPolicy,omitempty"`
}

const (
rrName = roundrobin.Name
rhName = ringhash.Name
Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough.

b := balancer.Get(Name)
if b == nil {
t.Fatalf("LB policy %q not registered", Name)
}
cfgParser, ok := b.(balancer.ConfigParser)
if !ok {
t.Fatalf("LB policy %q does not support config parsing", Name)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough. Doug left me that suggestion on my Outlier Detection PR mainly because I hadn't coded balancer implementation yet.

@zasweq zasweq removed their assignment Jun 24, 2022
@easwars easwars merged commit 4b75005 into grpc:master Jun 24, 2022
11 checks passed
@easwars easwars deleted the clusterresolver_parse_config branch July 15, 2022 17:23
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

xds: merge clusterresolver's exported and unexported P(p)arseConfig methods
2 participants