review comments

clientconn.go

@@ -31,6 +31,7 @@ import (
  "time"
 
  "google.golang.org/grpc/balancer"
+ "google.golang.org/grpc/balancer/base"
  _ "google.golang.org/grpc/balancer/roundrobin" // To register roundrobin.
  "google.golang.org/grpc/codes"
  "google.golang.org/grpc/connectivity"
@@ -586,10 +587,13 @@ func (cc *ClientConn) updateResolverState(s resolver.State, err error) error {
  if sc, ok := s.ServiceConfig.Config.(*ServiceConfig); s.ServiceConfig.Err == nil && ok {
  cc.applyServiceConfigAndBalancer(sc, s.Addresses)
  } else {
+ ret = balancer.ErrBadResolverState
  if cc.balancerWrapper == nil {
- cc.applyServiceConfigAndBalancer(emptyServiceConfig, s.Addresses)
+ cc.blockingpicker.updatePicker(base.NewErrPicker(status.Errorf(codes.Unavailable, "error parsing service config: %v", s.ServiceConfig.Err)))
+ cc.csMgr.updateState(connectivity.TransientFailure)
+ cc.mu.Unlock()
+ return ret
  }
- ret = balancer.ErrBadResolverState
  }
  }
 

service_config.go

@@ -265,13 +265,13 @@ func init() {
 }
 func parseServiceConfig(js string) *serviceconfig.ParseResult {
  if len(js) == 0 {
- return serviceconfig.NewParseResult(fmt.Errorf("no JSON service config provided"))
+ return &serviceconfig.ParseResult{Err: fmt.Errorf("no JSON service config provided")}
  }
  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.NewParseResult(err)
+ return &serviceconfig.ParseResult{Err: err}
  }
  sc := ServiceConfig{
  LB: rsc.LoadBalancingPolicy,
@@ -285,7 +285,7 @@ func parseServiceConfig(js string) *serviceconfig.ParseResult {
  if len(lbcfg) != 1 {
  err := fmt.Errorf("invalid loadBalancingConfig: entry %v does not contain exactly 1 policy/config pair: %q", i, lbcfg)
  grpclog.Warningf(err.Error())
- return serviceconfig.NewParseResult(err)
+ return &serviceconfig.ParseResult{Err: err}
  }
  var name string
  var jsonCfg json.RawMessage
@@ -300,7 +300,7 @@ func parseServiceConfig(js string) *serviceconfig.ParseResult {
  var err error
  sc.lbConfig.cfg, err = parser.ParseConfig(jsonCfg)
  if err != nil {
- return serviceconfig.NewParseResult(fmt.Errorf("error parsing loadBalancingConfig for policy %q: %v", name, err))
+ return &serviceconfig.ParseResult{Err: fmt.Errorf("error parsing loadBalancingConfig for policy %q: %v", name, err)}
  }
  } else if string(jsonCfg) != "{}" {
  grpclog.Warningf("non-empty balancer configuration %q, but balancer does not implement ParseConfig", string(jsonCfg))
@@ -313,12 +313,12 @@ func parseServiceConfig(js string) *serviceconfig.ParseResult {
  // case.
  err := fmt.Errorf("invalid loadBalancingConfig: no supported policies found")
  grpclog.Warningf(err.Error())
- return serviceconfig.NewParseResult(err)
+ return &serviceconfig.ParseResult{Err: err}
  }
  }
 
  if rsc.MethodConfig == nil {
- return serviceconfig.NewParseResult(&sc)
+ return &serviceconfig.ParseResult{Config: &sc}
  }
  for _, m := range *rsc.MethodConfig {
  if m.Name == nil {
@@ -327,7 +327,7 @@ func parseServiceConfig(js string) *serviceconfig.ParseResult {
  d, err := parseDuration(m.Timeout)
  if err != nil {
  grpclog.Warningf("grpc: parseServiceConfig error unmarshaling %s due to %v", js, err)
- return serviceconfig.NewParseResult(err)
+ return &serviceconfig.ParseResult{Err: err}
  }
 
  mc := MethodConfig{
@@ -336,7 +336,7 @@ func parseServiceConfig(js string) *serviceconfig.ParseResult {
  }
  if mc.retryPolicy, err = convertRetryPolicy(m.RetryPolicy); err != nil {
  grpclog.Warningf("grpc: parseServiceConfig error unmarshaling %s due to %v", js, err)
- return serviceconfig.NewParseResult(err)
+ return &serviceconfig.ParseResult{Err: err}
  }
  if m.MaxRequestMessageBytes != nil {
  if *m.MaxRequestMessageBytes > int64(maxInt) {
@@ -361,13 +361,13 @@ func parseServiceConfig(js string) *serviceconfig.ParseResult {
 
  if sc.retryThrottling != nil {
  if mt := sc.retryThrottling.MaxTokens; mt <= 0 || mt > 1000 {
- return serviceconfig.NewParseResult(fmt.Errorf("invalid retry throttling config: maxTokens (%v) out of range (0, 1000]", mt))
+ return &serviceconfig.ParseResult{Err: fmt.Errorf("invalid retry throttling config: maxTokens (%v) out of range (0, 1000]", mt)}
  }
  if tr := sc.retryThrottling.TokenRatio; tr <= 0 {
- return serviceconfig.NewParseResult(fmt.Errorf("invalid retry throttling config: tokenRatio (%v) may not be negative", tr))
+ return &serviceconfig.ParseResult{Err: fmt.Errorf("invalid retry throttling config: tokenRatio (%v) may not be negative", tr)}
  }
  }
- return serviceconfig.NewParseResult(&sc)
+ return &serviceconfig.ParseResult{Config: &sc}
 }
 
 func convertRetryPolicy(jrp *jsonRetryPolicy) (p *retryPolicy, err error) {

serviceconfig/serviceconfig.go

@@ -22,12 +22,6 @@
 // This package is EXPERIMENTAL.
 package serviceconfig
 
-import (
- "google.golang.org/grpc/codes"
- "google.golang.org/grpc/grpclog"
- "google.golang.org/grpc/status"
-)
-
 // Config represents an opaque data structure holding a service config.
 type Config interface {
  isServiceConfig()
@@ -39,21 +33,9 @@ type LoadBalancingConfig interface {
  isLoadBalancingConfig()
 }
 
-// ParseResult contains a service config or an error.
+// ParseResult contains a service config or an error. Exactly one must be
+// non-nil.
 type ParseResult struct {
  Config Config
  Err error
 }
-
-// NewParseResult returns a ParseResult returning the provided parameter as
-// either the Config or Err field, depending upon its type.
-func NewParseResult(configOrError interface{}) *ParseResult {
- if e, ok := configOrError.(error); ok {
- return &ParseResult{Err: e}
- }
- if c, ok := configOrError.(Config); ok {
- return &ParseResult{Config: c}
- }
- grpclog.Errorf("Unexpected configOrError type: %T", configOrError)
- return &ParseResult{Err: status.Errorf(codes.Internal, "unexpected configOrError type: %T", configOrError)}
-}