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

A46: xDS NACK Semantics Improvement #260

Merged
merged 5 commits into from Sep 20, 2021

Conversation

markdroth
Copy link
Member

@markdroth markdroth commented Sep 3, 2021

No description provided.

@markdroth markdroth requested review from ejona86 and dfawley Sep 3, 2021
ejona86
ejona86 approved these changes Sep 3, 2021
Copy link
Member

@ejona86 ejona86 left a comment

Beautiful. That was well documented.

dfawley
dfawley approved these changes Sep 3, 2021
Copy link
Contributor

@dfawley dfawley left a comment

A++++++++++++++++++ would read again.

A46-xds-nack-semantics-improvement.md Outdated Show resolved Hide resolved
@murgatroid99
Copy link
Member

murgatroid99 commented Sep 3, 2021

Should a NACK sent in this situation indicate which resources are rejected vs accepted in a machine-readable way (i.e. other than in the error message)?

@markdroth
Copy link
Member Author

markdroth commented Sep 3, 2021

@murgatroid99

Should a NACK sent in this situation indicate which resources are rejected vs accepted in a machine-readable way (i.e. other than in the error message)?

There's no way to do that today. That's what I was referring to in the doc by saying that there will be some future work to change the xDS protocol to make that possible.

Copy link
Contributor

@srini100 srini100 left a comment

Looks great! Thanks for taking care of this.

supports the `RING_HASH` policy, then it cannot safely send a Cluster
resource configuring that policy, because that change would cause older
clients to stop functioning.

Choose a reason for hiding this comment

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

won't this be addressed by minor version support on server side?

Copy link
Member Author

@markdroth markdroth Sep 7, 2021

Choose a reason for hiding this comment

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

We still don't actually have a design for version negotiation, so that's not a solution for the immediate problem. And even once we do have such a design, I suspect that that's not going to be the best way to address this kind of use-case, because it would require the control plane to have a bunch of logic to determine how to configure older clients that don't support the current configuration.

I think having the control plane generate different versions of the config will make sense for cases where we are deprecating one set of fields and replacing them with another set, where the semantic meaning of the underlying config can be expressed equally well via both sets of fields. But I think it will be too complex to do in cases where the client simply does not support the features that the configuration is intending to configure, because I don't think the control plane can really know what to do in that case.

dapengzhang0 added a commit to grpc/grpc-java that referenced this issue Sep 12, 2021
dapengzhang0 added a commit to dapengzhang0/grpc-java that referenced this issue Sep 13, 2021
dapengzhang0 added a commit to grpc/grpc-java that referenced this issue Sep 13, 2021
@markdroth markdroth merged commit 2f5f042 into grpc:master Sep 20, 2021
1 check passed
@markdroth markdroth deleted the xds_nack_semantics branch Sep 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants