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

xds/csds: populate new GenericXdsConfig field #4898

Merged
merged 4 commits into from Oct 26, 2021

Conversation

menghanl
Copy link
Contributor

@menghanl menghanl commented Oct 25, 2021

Related
grpc/grpc#27794
grpc/proposal#267
grpc-ecosystem/grpcdebug#12

RELEASE NOTES:

  • xds/csds: use new field GenericXdsConfig instead of PerXdsConfig

@menghanl menghanl added the Type: Behavior Change label Oct 25, 2021
@menghanl menghanl added this to the 1.42 Release milestone Oct 25, 2021
@menghanl menghanl requested a review from dfawley Oct 25, 2021
ret := &v3statuspb.ClientStatusResponse{
Config: []*v3statuspb.ClientConfig{
{
Node: nodeProtoToV3(s.xdsClient.BootstrapConfig().NodeProto),
XdsConfig: []*v3statuspb.PerXdsConfig{
Copy link
Contributor

@dfawley dfawley Oct 26, 2021

Choose a reason for hiding this comment

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

Should we keep setting this field to remain backward compatible? Did other languages do this, too?

Copy link
Contributor Author

@menghanl menghanl Oct 26, 2021

Choose a reason for hiding this comment

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

Copy link
Contributor

@dfawley dfawley Oct 26, 2021

Choose a reason for hiding this comment

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

Seems like a bad idea to me, but I assume almost nobody is using this yet, so at least there's that.

xds/csds/csds.go Outdated
Details: errState.Err.Error(),
VersionInfo: errState.Version,
}
func dumpToGenericXdsConfig(typeURL string, name string, d xdsclient.UpdateWithMD) *v3statuspb.ClientConfig_GenericXdsConfig {
Copy link
Contributor

@dfawley dfawley Oct 26, 2021

Choose a reason for hiding this comment

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

If this does even more, the callers can remove some duplicate logic. E.g.

func dumpToGenericXdsConfig(typeURL string, getUpdate func() (string, map[string]UpdateWithMD)) *v3statuspb.ClientConfig_GenericXdsConfig {
	...
}

func (s *ClientStatusDiscoveryServer) buildLDSPerXDSConfig() []*v3statuspb.ClientConfig_GenericXdsConfig {
	return dumpToGenericXdsConfig(listenerTypeURL, s.xdsClient.DumpLDS)
}

Copy link
Contributor Author

@menghanl menghanl Oct 26, 2021

Choose a reason for hiding this comment

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

Done. I deleted buildLDSPerXDSConfig() all together

@dfawley dfawley assigned menghanl and unassigned dfawley Oct 26, 2021
@menghanl menghanl assigned dfawley and unassigned menghanl Oct 26, 2021
a, b := out[i], out[j]
if a == nil {
return true
}
if b == nil {
return true
}
Copy link
Contributor

@dfawley dfawley Oct 26, 2021

Choose a reason for hiding this comment

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

Combine? if a == nil || b == nil { ... }

Copy link
Contributor Author

@menghanl menghanl Oct 26, 2021

Choose a reason for hiding this comment

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

b == nil should return false. Fixed

@dfawley dfawley assigned menghanl and unassigned dfawley Oct 26, 2021
@menghanl menghanl merged commit 9fa2698 into grpc:master Oct 26, 2021
10 checks passed
@menghanl menghanl deleted the csds_new_fields branch Oct 26, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Type: Behavior Change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants