-
Notifications
You must be signed in to change notification settings - Fork 4.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
docs: clarify xDS resource instance version semantics #12580
docs: clarify xDS resource instance version semantics #12580
Conversation
Signed-off-by: Mark D. Roth <[email protected]>
identifier. This holds true regardless of the acceptance of the discovery | ||
responses on the same stream. The node identifier should always be identical if | ||
present more than once on the stream. It is sufficient to only check the first | ||
message for the node identifier as a result. |
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.
fyi: this is broken for runtime type, found by accident. I think we may need to enforce it better in envoy before making the spec strict.
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.
This paragraph is actually not new; I just moved it from where it was before. This is already part of the spec, so if something is breaking that requirement, I think it's unrelated to this PR. (But I agree that this should be fixed.)
Signed-off-by: Mark D. Roth <[email protected]>
:ref:`DiscoveryRequest <envoy_api_msg_DiscoveryRequest>` from the client, specifying | ||
the list of resources to subscribe to, the type URL corresponding to the | ||
subscribed resources, the node identifier and an empty :ref:`version_info <envoy_api_field_DiscoveryRequest.version_info>`. | ||
Every xDS resource type has a version string that indicates the version for that resource type. |
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.
Do you want to add that this "resource type version" is the "system version" in delta 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.
Done.
Signed-off-by: Mark D. Roth <[email protected]>
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.
LGTM, thanks!
Clarify that the server may not resend resources upon stream restart only if it has some way to know that the client is not subscribing to new resources that it wasn't previously subscribed to. Risk Level: Low Testing: N/A Docs Changes: Included in PR Clarifies text added in #12580. Signed-off-by: Mark D. Roth <[email protected]>
) Clarify that the server may not resend resources upon stream restart only if it has some way to know that the client is not subscribing to new resources that it wasn't previously subscribed to. Risk Level: Low Testing: N/A Docs Changes: Included in PR Clarifies text added in envoyproxy#12580. Signed-off-by: Mark D. Roth <[email protected]> Signed-off-by: Craig Radcliffe <[email protected]>
Signed-off-by: Mark D. Roth [email protected]
Commit Message: docs: clarify xDS resource instance version semantics
Additional Description: Clarify that resource instance version can be reused across stream restarts. Also a few other small fixes and improvements.
Risk Level: Low
Testing: N/A
Docs Changes: Included in PR
Release Notes: N/A
CC @htuch @kyessenov