-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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: Preserve nonce when unsubscribing type #11796
Conversation
This fixes a regression introduced in 19c9b99. b/374697875
// The resource type no longer has subscribing resources; clean up references to it. | ||
// Except for nonces. If the resource type becomes used again the control plane can ignore | ||
// requests for old/missing nonces. Old type's nonces are dropped when the ADS stream is | ||
// restarted. | ||
versions.remove(resourceType); |
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.
I think we should retain the version also. The version doesn't change when we change the set of resources that we're subscribed to, and unsubscribing from all resources is just a special case of changing the set of resources that we're subscribed to.
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.
We had discussed this offline and Mark prefers keeping the version but doesn't see it as mandatory. We (grpc-java) want to drop our resource data structures, and that means the version is also lost. The version serves no purpose when there are no resources saved by the client, so it can't make any difference which way it is done.
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.
I agree that it probably won't cause any problems to drop it. But I do worry about potential edge cases we might hit in the future.
I guess let's kick the can down the road here and see if we have problems.
I'm purposefully waiting over the weekend before backporting. I want to see the tests go green first. |
This fixes a regression introduced in 19c9b99.
b/374697875