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

A67: xDS dynamic parameters #381

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

allanrbo
Copy link

No description provided.

@allanrbo allanrbo changed the title xDS dynamic parameters A67: xDS dynamic parameters Jul 21, 2023
Copy link
Member

@markdroth markdroth left a comment

Choose a reason for hiding this comment

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

Sorry for the delay on this! The doc looks good overall, comments are relatively minor.

Please let me know if you have any questions. Thanks!

A67-xds-dynamic-parameters.md Show resolved Hide resolved
A67-xds-dynamic-parameters.md Show resolved Hide resolved
A67-xds-dynamic-parameters.md Outdated Show resolved Hide resolved
A67-xds-dynamic-parameters.md Outdated Show resolved Hide resolved
* Status: Draft
* Implemented in:
* Last updated: 2023-07-21
* Discussion at:
Copy link
Member

Choose a reason for hiding this comment

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

Please go ahead and create the mailing list thread, and then add a link to it here, as per the gRFC process.

Copy link
Member

Choose a reason for hiding this comment

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

Still need to take care of this.

Copy link
Member

@markdroth markdroth left a comment

Choose a reason for hiding this comment

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

This looks really good!

@ejona86 @dfawley, I'd like reviews from you as well.

* Status: Draft
* Implemented in:
* Last updated: 2023-07-21
* Discussion at:
Copy link
Member

Choose a reason for hiding this comment

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

Still need to take care of this.

@@ -104,6 +104,9 @@ resources {

* CNCF xDS proposal
[TP2: Dynamically Generated Cacheable xDS Resources](https://github.com/cncf/xds/blob/main/proposals/TP2-dynamically-generated-cacheable-xds-resources.md).
* gRPC proposal[gRFC A47: xDS Federation](https://github.com/grpc/proposal/blob/master/A47-xds-federation.md).
Copy link
Member

Choose a reason for hiding this comment

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

I think we can remove the words "gRPC proposal". Anyone reading this already knows what a gRFC is. :)

Copy link
Member

@ejona86 ejona86 left a comment

Choose a reason for hiding this comment

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

This seems fine. Some comments to work out, but I'm not all that concerned how they are resolved.

...
foreach resource_names, dynamic_parameters in authority_resource_names
if dynamic_parameters is empty
request.resource_names = resource_names
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't this overwrite all but the last authority's resource names? Is this supposed to be add() as well like below? You either have to do two passes or mix both resource_names and resource_locators in a single request. It really isn't clear which way is intended here (based on the text above I thought only one of the two fields would have any entries, but the add() below makes me think maybe that wasn't the intention).


### No merging of top-level and per-authority dynamic parmeters

The `dynamic_parameters` at root level only apply to the non-xdstp case. There
Copy link
Member

Choose a reason for hiding this comment

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

That is not clear in the proposal text. It just says "To support non-xdstp use cases, we can add a map at the root level too". That doesn't require that it is used exclusively in non-xdstp.

I do wonder if there's something we can do here to avoid confusion where someone thinks they are configuring the dynamic parameters for xdstp, but actually aren't. However, I see the value in using the same dynamic_parameters name, just like we are doing with xds_servers. Although for xds_servers, if you don't specify it in the authorities section the top-level xds_servers is used. Maybe we should do that; it becomes the default, but there's still no merging. That's probably the least surprising. Although it does cause problems for disabling the dynamic parameters within authorities, because empty list is semantically the same as not present.

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.

3 participants