Implement/service config#2731
Conversation
29c707f to
52589c3
Compare
| 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?)
There was a problem hiding this comment.
Fixed the language here - LMK what you think.
| .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?
There was a problem hiding this comment.
I read the spec as requiring it, but re-reading it I think there are multiple ways to parse it. In any event, it appears that Golang does not support stringification, but Java and C++ do:
Java:
- Call site: https://github.com/grpc/grpc-java/blob/master/core/src/main/java/io/grpc/internal/ManagedChannelServiceConfig.java#L278
- Deserialization function: https://github.com/grpc/grpc-java/blob/master/core/src/main/java/io/grpc/internal/ServiceConfigUtil.java#L125-L258
C++:
- Call site: https://github.com/grpc/grpc/blob/master/src/core/lib/channel/message_size_filter.cc#L74-L78
- Deserialization function: https://github.com/grpc/grpc/blob/master/src/core/util/json/json_object_loader.cc#L29-L41
I think in the original design it deferred to the C++ implementation. Let me know if you want to pull this, however, and we can only accept pure numbers.
|
|
||
| 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)
There was a problem hiding this comment.
I've gone ahead and implemented this. We didn't talk about it in the design stage, but it seems like a good idea. That said, take a look and let me know if that fits properly.
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.