grpc: change the LB policy config parsing API to accept JSON values instead of String#2294
grpc: change the LB policy config parsing API to accept JSON values instead of String#2294dfawley merged 4 commits intohyperium:nextfrom
Conversation
| /// JSON. Hides internal storage details and includes a method to deserialize | ||
| /// the JSON into a concrete policy struct. | ||
| #[derive(Debug)] | ||
| pub struct ParsedJsonLbConfig(pub serde_json::Value); |
There was a problem hiding this comment.
| pub struct ParsedJsonLbConfig(pub serde_json::Value); | |
| pub struct ParsedJsonLbConfig(serde_json::Value); |
The idea is to encapsulate serde_json and not expose it outside our crate.
There was a problem hiding this comment.
Currently the channel (in the gracefulswitch LB policy) specifies a default service config (with pick_first as the LB policy). So, we have a few options:
- Make this field
pub(super)so that the channel has access to this field. Also, maybe the channel would want this when it implements the service config parsing from the resolver? - Make this field
pub(crate)if other tests end wanting to set a service config. - Have an associated function
newthat takes the input JSON as a string, and returnsResult<Self, String>.- we could use the
serde_json::from_strfunction to convert the input string into aserde_json::Value
- we could use the
What do you think?
There was a problem hiding this comment.
New function makes the most sense or you can even implement FromStr, exposing the internal type to modules outside of this file is generally a bad idea. (eg you want to add a field to this struct then you need to refactor all the locations that reference the internals.
There was a problem hiding this comment.
Moved the value to be a struct field instead of this being a tuple struct.
Also, added a new function.
|
Gentle ping on this PR: @LucioFranco @dfawley |
3 participants
The existing
parse_configmethod on theLbPolicyBuildertrait accepts configuration as a&str. Passing parsed JSON to this API is better for the following reasons: