service config: default service config #2686
Conversation
| // will be used in cases where: | ||
| // 1. WithDisableServiceConfig is called. | ||
| // 2. Resolver does not return service config or if the resolver gets and invalid config. | ||
| func WithDefaultServiceConfig(s string) DialOption { |
There was a problem hiding this comment.
As mentioned over GVC, could we add a link or some documentation describing what this string should look like?
There was a problem hiding this comment.
WIP to export a doc about canonical service config json string.
clientconn.go
Outdated
| return nil | ||
| } | ||
| // apply default service config when there's resolver service config is disabled. | ||
| return cc.applyServiceConfig(cc.dopts.defaultServiceConfig, cc.dopts.defaultServiceConfigRaw) |
There was a problem hiding this comment.
This will be called every time handleServiceConfig is called
clientconn.go
Outdated
| if err != nil { | ||
| return err | ||
| } | ||
| sc = &scfg |
There was a problem hiding this comment.
return pointer from parseServiceConfig?
dialoptions.go
Outdated
| disableHealthCheck bool | ||
| healthCheckFunc internal.HealthChecker | ||
| defaultServiceConfig *ServiceConfig | ||
| defaultServiceConfigRaw string |
There was a problem hiding this comment.
Add a field to ServiceConfig for the raw js string.
dialoptions.go
Outdated
| // 2. Resolver does not return service config or if the resolver gets and invalid config. | ||
| func WithDefaultServiceConfig(s string) DialOption { | ||
| return newFuncDialOption(func(o *dialOptions) { | ||
| sc, err := parseServiceConfig(s) |
There was a problem hiding this comment.
We still give users a chance to pass invalid string here.
How about returning a dial option and an error from ValidateServiceConfig(), so the only way to create a default service config dial option is to validate it first?
The name ValidateServiceConfig would be inappropriate, time to bikeshed.
There was a problem hiding this comment.
If we have something like
func WithDefaultServiceConfig(s string) (DialOption, error) {
sc, err := parseServiceConfig(s)
if err != nil {
return nil, fmt.Errorf("the provided service config is invalid, err: %v", err)
}
return newFuncDialOption(func(o *dialOptions) {
o.defaultServiceConfig = sc
}), nil
}
Then it breaks the idiomatic way of doing grpc.Dial(target, grpc.WithXXX(), grpc.WithYYY()). I think separating the validation is ok. I can add a comment saying that it's strongly recommended to verify the validity of the json string with the ValidateServiceConfig function.
There was a problem hiding this comment.
Per offline discussion, we will check the validity of the default service config in DialContext. If it is not valid, the Dial will fail.
lyuxuan
force-pushed
the
defsc
branch
6 times, most recently
from
b44a9c0 to
9fab3e0
Compare
No description provided.