A67: xDS dynamic parameters #381
Conversation
| * Status: Draft | ||
| * Implemented in: | ||
| * Last updated: 2023-07-21 | ||
| * Discussion at: |
There was a problem hiding this comment.
Please go ahead and create the mailing list thread, and then add a link to it here, as per the gRFC process.
There was a problem hiding this comment.
Still need to take care of this.
| * Status: Draft | ||
| * Implemented in: | ||
| * Last updated: 2023-07-21 | ||
| * Discussion at: |
There was a problem hiding this comment.
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). | |||
There was a problem hiding this comment.
I think we can remove the words "gRPC proposal". Anyone reading this already knows what a gRFC is. :)
| ... | ||
| foreach resource_names, dynamic_parameters in authority_resource_names | ||
| if dynamic_parameters is empty | ||
| request.resource_names = resource_names |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
No description provided.