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

Implementation of the xds_experimental resolver. #2967

Merged
merged 1 commit into from Aug 21, 2019

Conversation

easwars
Copy link
Contributor

@easwars easwars commented Aug 12, 2019

This resolver doesn't do much at this point, except returning an empty
address list and a hard-coded service config which picks the xds
balancer with a round_robin child policy.

Also moved the xdsConfig struct to the xds/internal package and exported
it as LBConfig, so that both the resolver and the balancer packages can
make use of this.


This change is Reviewable

@easwars easwars requested a review from menghanl August 12, 2019 17:17
@easwars
Copy link
Contributor Author

easwars commented Aug 15, 2019

Removed balancerName field from the hard-coded service config.

xds/experimental/xds_experimental.go Outdated Show resolved Hide resolved
xds/internal/resolver/xds_resolver.go Outdated Show resolved Hide resolved
parseOnce.Do(func() {
// The xds balancer must have been registered at this point for the service
// config to be parsed properly.
psc, err := internal.ParseServiceConfig(jsonSC)
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, you have to move this to Build after #2951 because internal.ParseServiceConfig() will become cc.ParseServiceConfig()...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm ... I've left it as is for now. Whichever one goes in second has to resolve the conflict I guess :)

"google.golang.org/grpc/xds/internal/balancer/edsbalancer"
"google.golang.org/grpc/xds/internal/balancer/lrs"
cdspb "google.golang.org/grpc/xds/internal/proto/envoy/api/v2/cds"
edspb "google.golang.org/grpc/xds/internal/proto/envoy/api/v2/eds"
)

const (
defaultTimeout = 10 * time.Second
xdsName = "xds_experimental"
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually let's keep the names separate....

Underscore is not a valid character for URLs. So the resolver will use xds-experimental, and balancer will keep using xds_experimental..

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. PTAL.

Copy link
Contributor

Choose a reason for hiding this comment

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

Balancer name should remain "xds_experimental"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad. Fixed it now.

defer r.Close()

<-tcc.done
if len(tcc.gotState.Addresses) != 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you want to also compare the service config?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem here is that the lbConfig field in the service config is not exported. So, we really can't compare here. But we do make sure that the service config is as expected in TestXDSRsolverServiceConfig.

}
defer cc.Close()

timeout := time.After(5 * time.Second)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: this may leak for 5 seconds.
Make a timer and close?

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. Thanks.

This resolver doesn't do much at this point, except returning an empty
address list and a hard-coded service config which picks the xds
balancer with a round_robin child policy.

Also moved the xdsConfig struct to the xds/internal package and exported
it as LBConfig, so that both the resolver and the balancer packages can
make use of this.
@easwars easwars merged commit dc18754 into grpc:master Aug 21, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Feb 17, 2020
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.

None yet

2 participants