Implement/service config#2731
Conversation
29c707f to
52589c3
Compare
| use serde::Deserialize; | ||
| use std::time::Duration; |
There was a problem hiding this comment.
Nit: please try to follow import groupings: std, external, internal.
| use serde::Deserialize; | ||
| use std::time::Duration; | ||
|
|
||
| // See [service config format notes](https://protobuf.dev/reference/protobuf/google.protobuf/#duration) |
There was a problem hiding this comment.
The link text here may be misleading. The link itself is to the proto duration parsing page, not service config notes.
Also, without triple slashes, I don't think this ever renders as a link anyway? (Probably should triple-slash this?)
|
|
||
| assert!(parse_duration("").is_err()); | ||
| assert!(parse_duration("1").is_err()); | ||
| assert!(parse_duration("1.s").is_err()); |
There was a problem hiding this comment.
Is ".5s" valid or an error? Please add a test for it.
| .transpose() | ||
| } | ||
|
|
||
| // This included to allow deserializing both u32 and string (or "stringified") |
There was a problem hiding this comment.
| // This included to allow deserializing both u32 and string (or "stringified") | |
| // This is included to allow deserializing both u32 and string (or "stringified") |
?
| // representations of u32, as well as null values. This is used for the | ||
| // `max_concurrent_streams` field in the service config. |
There was a problem hiding this comment.
We don't have a max_concurrent_streams in service config, do we?
| .transpose() | ||
| } | ||
|
|
||
| // This included to allow deserializing both u32 and string (or "stringified") |
There was a problem hiding this comment.
Why are we supporting stringified numbers? Is that a requirement?
|
|
||
| impl ServiceConfig { | ||
| /// Parses a service configuration from a JSON string. | ||
| pub fn parse(config_json: &str) -> Result<Self, String> { |
There was a problem hiding this comment.
I believe we might want a service_config::ParseResult type here that is Result<ServiceConfig, String>, for newtype reasons. The name resolve will be passing the full ParseResult to the channel.
(Go reference: https://pkg.go.dev/google.golang.org/grpc@v1.82.1/serviceconfig#ParseResult + passed to channel with https://pkg.go.dev/google.golang.org/grpc/resolver#State)
| .is_some_and(|lbc| lbc.iter().any(|c| c.name == "round_robin")) | ||
| { | ||
| json!([{round_robin::POLICY_NAME: {}}]) |
There was a problem hiding this comment.
I think we can delete the first part of this if and just use pick_first if there isn't an LB policy in the config at all. Unless you're saving that for the next PR because it cases even more changes?
| json!([{pick_first::POLICY_NAME: {"shuffleAddressList": true, "unknown_field": false}}]) | ||
| }; | ||
|
|
||
| // TODO: config should come from ServiceConfig. |
There was a problem hiding this comment.
This can be deleted if you do the above?
| }, | ||
| ]), | ||
| method_config: None, | ||
| retry_throttling: None, |
There was a problem hiding this comment.
Can this use the channel_controller.parse_service_config on a string instead?
2 participants
Motivation
ServiceConfig requires an internal representation that we can de/serialize.
Solution
This CL implements the ServiceConfig struct and necessary plumbing for de/serialization. In a follow-on CL(s) we will be adding the plumbing to fully utilize the ServiceConfig, including adding the option to the
Channelbuilde, passing it to the load balancer tree for utilization, and interactions with the Name Resolver.