From 47bb6ca524e1d4aa239c17558ff3bf36f0c9f12c Mon Sep 17 00:00:00 2001 From: Yuxuan Li Date: Tue, 19 Mar 2019 17:08:49 -0700 Subject: [PATCH] fix review comments --- clientconn.go | 16 ++++++++-------- dialoptions.go | 22 +++++++++++----------- service_config.go | 17 +++++++++++------ service_config_test.go | 16 +++++++++++----- 4 files changed, 41 insertions(+), 30 deletions(-) diff --git a/clientconn.go b/clientconn.go index 34c4da29d69b..b6f42a516cff 100644 --- a/clientconn.go +++ b/clientconn.go @@ -439,7 +439,7 @@ func (cc *ClientConn) scWatcher() { // We may revisit this decision in the future. cc.sc = sc s := "" - cc.scRaw = &s + cc.sc.rawJSONString = &s cc.mu.Unlock() case <-cc.ctx.Done(): return @@ -479,8 +479,8 @@ func (cc *ClientConn) handleResolvedAddrs(addrs []resolver.Address, err error) { } // If resolver does not return service config, apply the default service config if available. - if cc.scRaw == nil && cc.dopts.defaultServiceConfig != nil { - cc.applyServiceConfig(cc.dopts.defaultServiceConfig, cc.dopts.defaultServiceConfigRaw) + if cc.sc.rawJSONString == nil && cc.dopts.defaultServiceConfig != nil { + cc.applyServiceConfig(cc.dopts.defaultServiceConfig, *cc.dopts.defaultServiceConfig.rawJSONString) } cc.curAddresses = addrs @@ -770,11 +770,11 @@ func (cc *ClientConn) handleServiceConfig(js string) error { cc.mu.Lock() defer cc.mu.Unlock() if cc.dopts.disableServiceConfig { - if cc.dopts.defaultServiceConfig == nil { + if cc.dopts.defaultServiceConfig == nil || cc.scRaw != nil { return nil } // apply default service config when there's resolver service config is disabled. - return cc.applyServiceConfig(cc.dopts.defaultServiceConfig, cc.dopts.defaultServiceConfigRaw) + return cc.applyServiceConfig(cc.dopts.defaultServiceConfig, *cc.dopts.defaultServiceConfig.rawJSONString) } return cc.applyServiceConfig(nil, js) @@ -784,7 +784,7 @@ func (cc *ClientConn) handleServiceConfig(js string) error { // a parsed service config, we will skip the parsing. It will also skip the whole processing if // the new service config is the same as the old one. func (cc *ClientConn) applyServiceConfig(sc *ServiceConfig, js string) error { - if cc.scRaw != nil && *cc.scRaw == js { + if cc.sc.rawJSONString != nil && *cc.sc.rawJSONString == js { return nil } @@ -794,7 +794,7 @@ func (cc *ClientConn) applyServiceConfig(sc *ServiceConfig, js string) error { if err != nil { return err } - sc = &scfg + sc = scfg } if channelz.IsOn() { @@ -811,7 +811,7 @@ func (cc *ClientConn) applyServiceConfig(sc *ServiceConfig, js string) error { if cc.conns == nil { return nil } - cc.scRaw = &js + sc.rawJSONString = &js cc.sc = *sc if sc.retryThrottling != nil { diff --git a/dialoptions.go b/dialoptions.go index 556c9e70af3f..024ba2725a40 100644 --- a/dialoptions.go +++ b/dialoptions.go @@ -55,15 +55,14 @@ type dialOptions struct { // balancer, and also by WithBalancerName dial option. balancerBuilder balancer.Builder // This is to support grpclb. - resolverBuilder resolver.Builder - reqHandshake envconfig.RequireHandshakeSetting - channelzParentID int64 - disableServiceConfig bool - disableRetry bool - disableHealthCheck bool - healthCheckFunc internal.HealthChecker - defaultServiceConfig *ServiceConfig - defaultServiceConfigRaw string + resolverBuilder resolver.Builder + reqHandshake envconfig.RequireHandshakeSetting + channelzParentID int64 + disableServiceConfig bool + disableRetry bool + disableHealthCheck bool + healthCheckFunc internal.HealthChecker + defaultServiceConfig *ServiceConfig } // DialOption configures how we set up the connection. @@ -452,6 +451,8 @@ func WithDisableServiceConfig() DialOption { // 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. +// It is strongly recommended that caller of this function verifies the validity of the input string +// by using the grpc.ValidateServiceConfig function. func WithDefaultServiceConfig(s string) DialOption { return newFuncDialOption(func(o *dialOptions) { sc, err := parseServiceConfig(s) @@ -459,8 +460,7 @@ func WithDefaultServiceConfig(s string) DialOption { grpclog.Warningf("the provided service config is invalid, err: %v", err) return } - o.defaultServiceConfig = &sc - o.defaultServiceConfigRaw = s + o.defaultServiceConfig = sc }) } diff --git a/service_config.go b/service_config.go index 526fb55a3d5c..0f2ffc64c2fa 100644 --- a/service_config.go +++ b/service_config.go @@ -99,6 +99,10 @@ type ServiceConfig struct { // healthCheckConfig must be set as one of the requirement to enable LB channel // health check. healthCheckConfig *healthCheckConfig + // rawJSONString stores the pointer to the original service config json string that get parsed into + // this service config struct. Null pointer means this is a service config struct just defined, + // but no json string has been parsed and initialize the struct. + rawJSONString *string } // healthCheckConfig defines the go-native version of the LB channel health check config. @@ -238,12 +242,12 @@ type jsonSC struct { HealthCheckConfig *healthCheckConfig } -func parseServiceConfig(js string) (ServiceConfig, error) { +func parseServiceConfig(js string) (*ServiceConfig, error) { var rsc jsonSC err := json.Unmarshal([]byte(js), &rsc) if err != nil { grpclog.Warningf("grpc: parseServiceConfig error unmarshaling %s due to %v", js, err) - return ServiceConfig{}, err + return &ServiceConfig{}, err } sc := ServiceConfig{ LB: rsc.LoadBalancingPolicy, @@ -252,7 +256,7 @@ func parseServiceConfig(js string) (ServiceConfig, error) { healthCheckConfig: rsc.HealthCheckConfig, } if rsc.MethodConfig == nil { - return sc, nil + return &sc, nil } for _, m := range *rsc.MethodConfig { @@ -262,7 +266,7 @@ func parseServiceConfig(js string) (ServiceConfig, error) { d, err := parseDuration(m.Timeout) if err != nil { grpclog.Warningf("grpc: parseServiceConfig error unmarshaling %s due to %v", js, err) - return ServiceConfig{}, err + return &ServiceConfig{}, err } mc := MethodConfig{ @@ -271,7 +275,7 @@ func parseServiceConfig(js string) (ServiceConfig, error) { } if mc.retryPolicy, err = convertRetryPolicy(m.RetryPolicy); err != nil { grpclog.Warningf("grpc: parseServiceConfig error unmarshaling %s due to %v", js, err) - return ServiceConfig{}, err + return &ServiceConfig{}, err } if m.MaxRequestMessageBytes != nil { if *m.MaxRequestMessageBytes > int64(maxInt) { @@ -302,7 +306,8 @@ func parseServiceConfig(js string) (ServiceConfig, error) { sc.retryThrottling = nil } } - return sc, nil + sc.rawJSONString = &js + return &sc, nil } func convertRetryPolicy(jrp *jsonRetryPolicy) (p *retryPolicy, err error) { diff --git a/service_config_test.go b/service_config_test.go index 020b643f89c8..18c81eeb121e 100644 --- a/service_config_test.go +++ b/service_config_test.go @@ -79,8 +79,8 @@ func (s) TestParseLoadBalancer(t *testing.T) { for _, c := range testcases { sc, err := parseServiceConfig(c.scjs) - if c.wantErr != (err != nil) || !reflect.DeepEqual(sc, c.wantSC) { - t.Fatalf("parseServiceConfig(%s) = %+v, %v, want %+v, %v", c.scjs, sc, err, c.wantSC, c.wantErr) + if c.wantErr != (err != nil) || !scCompareWithRawJSONSkipped(*sc, c.wantSC) { + t.Fatalf("parseServiceConfig(%s) = %+v, %v, want %+v, %v", c.scjs, *sc, err, c.wantSC, c.wantErr) } } } @@ -167,7 +167,7 @@ func (s) TestParseWaitForReady(t *testing.T) { for _, c := range testcases { sc, err := parseServiceConfig(c.scjs) - if c.wantErr != (err != nil) || !reflect.DeepEqual(sc, c.wantSC) { + if c.wantErr != (err != nil) || !scCompareWithRawJSONSkipped(*sc, c.wantSC) { t.Fatalf("parseServiceConfig(%s) = %+v, %v, want %+v, %v", c.scjs, sc, err, c.wantSC, c.wantErr) } } @@ -249,7 +249,7 @@ func (s) TestPraseTimeOut(t *testing.T) { for _, c := range testcases { sc, err := parseServiceConfig(c.scjs) - if c.wantErr != (err != nil) || !reflect.DeepEqual(sc, c.wantSC) { + if c.wantErr != (err != nil) || !scCompareWithRawJSONSkipped(*sc, c.wantSC) { t.Fatalf("parseServiceConfig(%s) = %+v, %v, want %+v, %v", c.scjs, sc, err, c.wantSC, c.wantErr) } } @@ -318,7 +318,7 @@ func (s) TestPraseMsgSize(t *testing.T) { for _, c := range testcases { sc, err := parseServiceConfig(c.scjs) - if c.wantErr != (err != nil) || !reflect.DeepEqual(sc, c.wantSC) { + if c.wantErr != (err != nil) || !scCompareWithRawJSONSkipped(*sc, c.wantSC) { t.Fatalf("parseServiceConfig(%s) = %+v, %v, want %+v, %v", c.scjs, sc, err, c.wantSC, c.wantErr) } } @@ -384,3 +384,9 @@ func newDuration(b time.Duration) *time.Duration { func newString(b string) *string { return &b } + +func scCompareWithRawJSONSkipped(s1, s2 ServiceConfig) bool { + s1.rawJSONString = nil + s2.rawJSONString = nil + return reflect.DeepEqual(s1, s2) +}