-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
resolver: State: add Endpoints and deprecate Addresses #6471
Conversation
I see that this is being used in google3. |
Argh, thanks for checking. I think we can just remove this usage as well. There doesn't seem to be any harm to leaving a test resolver registered in the case there was no other resolver with that name registered before. Also that test imports "xds" so that resolver is guaranteed to be registered already, and we will always replace it with |
// GRPCResolverSchemeExtraMetadata determines when gRPC will add extra | ||
// metadata to RPCs. | ||
GRPCResolverSchemeExtraMetadata string = "xds" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this supposed to be only for the xds
scheme or do we plan to add this for the xdstp
scheme as well?
Also, could you help me figure out how this is used. The code in stream.go
adds the metadata, but I don't see how or where this is used. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we supporting xdstp
for the target string? If so, this needs to be extended.
This is read here when doing metadata matching for routes:
if extraMD, ok := grpcutil.ExtraMetadata(info.Context); ok { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we supporting
xdstp
for the target string?
Hmm ... A47 section on target-uri syntax says we will continue to support the xds
scheme for convenience. It does not say that we will start supporting the xdstp
scheme. Nor does it say we will not support the xdstp
scheme and instead the translations will be handled by the bootstrap configuration.
Do you think we should improve the language in this gRFC section or is it clear to you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not super air-tight, but by not saying "we're supporting xdstp
as a gRPC name resolver scheme and here's how", then I wouldn't assume we are adding it. We'd need a spec for how to handle it as a channel-level target. The text that follows does describe how we'll support this new stuff in the existing xds
resolver instead of adding a top-level resolver scheme for xdstp
:
we want the deployment to be able to decide to use new-style resource names without requiring users to explicitly specify them. To that end, we will add support for an optional authority in the
xds
URI. The procedure for handling anxds
URI is as follows:
// GRPCResolverSchemeExtraMetadata determines when gRPC will add extra | ||
// metadata to RPCs. | ||
GRPCResolverSchemeExtraMetadata string = "xds" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we supporting
xdstp
for the target string?
Hmm ... A47 section on target-uri syntax says we will continue to support the xds
scheme for convenience. It does not say that we will start supporting the xdstp
scheme. Nor does it say we will not support the xdstp
scheme and instead the translations will be handled by the bootstrap configuration.
Do you think we should improve the language in this gRFC section or is it clear to you?
Also:
resolver.UnregisterForTesting
, which we didn't need.internal.GRPCResolverSchemeExtraMetadata
to control the behavior of setting additional metadata in the context for the user agent string, which is needed by xDS routing code.Even though this is backward compatible and removes nothing, we should probably wait until several LB policies have been converted over to read
Endpoints
instead ofAddresses
before merging this PR. I'll be working on that in the meantime.RELEASE NOTES: