Skip to content

Commit 47bb6ca

Browse files
lyuxuanlyuxuan
committed
fix review comments
1 parent f0112c1 commit 47bb6ca

File tree

4 files changed

+41
-30
lines changed

4 files changed

+41
-30
lines changed

Diff for: ‎clientconn.go

+8-8
Original file line numberDiff line numberDiff line change
@@ -439,7 +439,7 @@ func (cc *ClientConn) scWatcher() {
439439
// We may revisit this decision in the future.
440440
cc.sc = sc
441441
s := ""
442-
cc.scRaw = &s
442+
cc.sc.rawJSONString = &s
443443
cc.mu.Unlock()
444444
case <-cc.ctx.Done():
445445
return
@@ -479,8 +479,8 @@ func (cc *ClientConn) handleResolvedAddrs(addrs []resolver.Address, err error) {
479479
}
480480

481481
// If resolver does not return service config, apply the default service config if available.
482-
if cc.scRaw == nil && cc.dopts.defaultServiceConfig != nil {
483-
cc.applyServiceConfig(cc.dopts.defaultServiceConfig, cc.dopts.defaultServiceConfigRaw)
482+
if cc.sc.rawJSONString == nil && cc.dopts.defaultServiceConfig != nil {
483+
cc.applyServiceConfig(cc.dopts.defaultServiceConfig, *cc.dopts.defaultServiceConfig.rawJSONString)
484484
}
485485

486486
cc.curAddresses = addrs
@@ -770,11 +770,11 @@ func (cc *ClientConn) handleServiceConfig(js string) error {
770770
cc.mu.Lock()
771771
defer cc.mu.Unlock()
772772
if cc.dopts.disableServiceConfig {
773-
if cc.dopts.defaultServiceConfig == nil {
773+
if cc.dopts.defaultServiceConfig == nil || cc.scRaw != nil {
774774
return nil
775775
}
776776
// apply default service config when there's resolver service config is disabled.
777-
return cc.applyServiceConfig(cc.dopts.defaultServiceConfig, cc.dopts.defaultServiceConfigRaw)
777+
return cc.applyServiceConfig(cc.dopts.defaultServiceConfig, *cc.dopts.defaultServiceConfig.rawJSONString)
778778
}
779779

780780
return cc.applyServiceConfig(nil, js)
@@ -784,7 +784,7 @@ func (cc *ClientConn) handleServiceConfig(js string) error {
784784
// a parsed service config, we will skip the parsing. It will also skip the whole processing if
785785
// the new service config is the same as the old one.
786786
func (cc *ClientConn) applyServiceConfig(sc *ServiceConfig, js string) error {
787-
if cc.scRaw != nil && *cc.scRaw == js {
787+
if cc.sc.rawJSONString != nil && *cc.sc.rawJSONString == js {
788788
return nil
789789
}
790790

@@ -794,7 +794,7 @@ func (cc *ClientConn) applyServiceConfig(sc *ServiceConfig, js string) error {
794794
if err != nil {
795795
return err
796796
}
797-
sc = &scfg
797+
sc = scfg
798798
}
799799

800800
if channelz.IsOn() {
@@ -811,7 +811,7 @@ func (cc *ClientConn) applyServiceConfig(sc *ServiceConfig, js string) error {
811811
if cc.conns == nil {
812812
return nil
813813
}
814-
cc.scRaw = &js
814+
sc.rawJSONString = &js
815815
cc.sc = *sc
816816

817817
if sc.retryThrottling != nil {

Diff for: ‎dialoptions.go

+11-11
Original file line numberDiff line numberDiff line change
@@ -55,15 +55,14 @@ type dialOptions struct {
5555
// balancer, and also by WithBalancerName dial option.
5656
balancerBuilder balancer.Builder
5757
// This is to support grpclb.
58-
resolverBuilder resolver.Builder
59-
reqHandshake envconfig.RequireHandshakeSetting
60-
channelzParentID int64
61-
disableServiceConfig bool
62-
disableRetry bool
63-
disableHealthCheck bool
64-
healthCheckFunc internal.HealthChecker
65-
defaultServiceConfig *ServiceConfig
66-
defaultServiceConfigRaw string
58+
resolverBuilder resolver.Builder
59+
reqHandshake envconfig.RequireHandshakeSetting
60+
channelzParentID int64
61+
disableServiceConfig bool
62+
disableRetry bool
63+
disableHealthCheck bool
64+
healthCheckFunc internal.HealthChecker
65+
defaultServiceConfig *ServiceConfig
6766
}
6867

6968
// DialOption configures how we set up the connection.
@@ -452,15 +451,16 @@ func WithDisableServiceConfig() DialOption {
452451
// will be used in cases where:
453452
// 1. WithDisableServiceConfig is called.
454453
// 2. Resolver does not return service config or if the resolver gets and invalid config.
454+
// It is strongly recommended that caller of this function verifies the validity of the input string
455+
// by using the grpc.ValidateServiceConfig function.
455456
func WithDefaultServiceConfig(s string) DialOption {
456457
return newFuncDialOption(func(o *dialOptions) {
457458
sc, err := parseServiceConfig(s)
458459
if err != nil {
459460
grpclog.Warningf("the provided service config is invalid, err: %v", err)
460461
return
461462
}
462-
o.defaultServiceConfig = &sc
463-
o.defaultServiceConfigRaw = s
463+
o.defaultServiceConfig = sc
464464
})
465465
}
466466

Diff for: ‎service_config.go

+11-6
Original file line numberDiff line numberDiff line change
@@ -99,6 +99,10 @@ type ServiceConfig struct {
9999
// healthCheckConfig must be set as one of the requirement to enable LB channel
100100
// health check.
101101
healthCheckConfig *healthCheckConfig
102+
// rawJSONString stores the pointer to the original service config json string that get parsed into
103+
// this service config struct. Null pointer means this is a service config struct just defined,
104+
// but no json string has been parsed and initialize the struct.
105+
rawJSONString *string
102106
}
103107

104108
// healthCheckConfig defines the go-native version of the LB channel health check config.
@@ -238,12 +242,12 @@ type jsonSC struct {
238242
HealthCheckConfig *healthCheckConfig
239243
}
240244

241-
func parseServiceConfig(js string) (ServiceConfig, error) {
245+
func parseServiceConfig(js string) (*ServiceConfig, error) {
242246
var rsc jsonSC
243247
err := json.Unmarshal([]byte(js), &rsc)
244248
if err != nil {
245249
grpclog.Warningf("grpc: parseServiceConfig error unmarshaling %s due to %v", js, err)
246-
return ServiceConfig{}, err
250+
return &ServiceConfig{}, err
247251
}
248252
sc := ServiceConfig{
249253
LB: rsc.LoadBalancingPolicy,
@@ -252,7 +256,7 @@ func parseServiceConfig(js string) (ServiceConfig, error) {
252256
healthCheckConfig: rsc.HealthCheckConfig,
253257
}
254258
if rsc.MethodConfig == nil {
255-
return sc, nil
259+
return &sc, nil
256260
}
257261

258262
for _, m := range *rsc.MethodConfig {
@@ -262,7 +266,7 @@ func parseServiceConfig(js string) (ServiceConfig, error) {
262266
d, err := parseDuration(m.Timeout)
263267
if err != nil {
264268
grpclog.Warningf("grpc: parseServiceConfig error unmarshaling %s due to %v", js, err)
265-
return ServiceConfig{}, err
269+
return &ServiceConfig{}, err
266270
}
267271

268272
mc := MethodConfig{
@@ -271,7 +275,7 @@ func parseServiceConfig(js string) (ServiceConfig, error) {
271275
}
272276
if mc.retryPolicy, err = convertRetryPolicy(m.RetryPolicy); err != nil {
273277
grpclog.Warningf("grpc: parseServiceConfig error unmarshaling %s due to %v", js, err)
274-
return ServiceConfig{}, err
278+
return &ServiceConfig{}, err
275279
}
276280
if m.MaxRequestMessageBytes != nil {
277281
if *m.MaxRequestMessageBytes > int64(maxInt) {
@@ -302,7 +306,8 @@ func parseServiceConfig(js string) (ServiceConfig, error) {
302306
sc.retryThrottling = nil
303307
}
304308
}
305-
return sc, nil
309+
sc.rawJSONString = &js
310+
return &sc, nil
306311
}
307312

308313
func convertRetryPolicy(jrp *jsonRetryPolicy) (p *retryPolicy, err error) {

Diff for: ‎service_config_test.go

+11-5
Original file line numberDiff line numberDiff line change
@@ -79,8 +79,8 @@ func (s) TestParseLoadBalancer(t *testing.T) {
7979

8080
for _, c := range testcases {
8181
sc, err := parseServiceConfig(c.scjs)
82-
if c.wantErr != (err != nil) || !reflect.DeepEqual(sc, c.wantSC) {
83-
t.Fatalf("parseServiceConfig(%s) = %+v, %v, want %+v, %v", c.scjs, sc, err, c.wantSC, c.wantErr)
82+
if c.wantErr != (err != nil) || !scCompareWithRawJSONSkipped(*sc, c.wantSC) {
83+
t.Fatalf("parseServiceConfig(%s) = %+v, %v, want %+v, %v", c.scjs, *sc, err, c.wantSC, c.wantErr)
8484
}
8585
}
8686
}
@@ -167,7 +167,7 @@ func (s) TestParseWaitForReady(t *testing.T) {
167167

168168
for _, c := range testcases {
169169
sc, err := parseServiceConfig(c.scjs)
170-
if c.wantErr != (err != nil) || !reflect.DeepEqual(sc, c.wantSC) {
170+
if c.wantErr != (err != nil) || !scCompareWithRawJSONSkipped(*sc, c.wantSC) {
171171
t.Fatalf("parseServiceConfig(%s) = %+v, %v, want %+v, %v", c.scjs, sc, err, c.wantSC, c.wantErr)
172172
}
173173
}
@@ -249,7 +249,7 @@ func (s) TestPraseTimeOut(t *testing.T) {
249249

250250
for _, c := range testcases {
251251
sc, err := parseServiceConfig(c.scjs)
252-
if c.wantErr != (err != nil) || !reflect.DeepEqual(sc, c.wantSC) {
252+
if c.wantErr != (err != nil) || !scCompareWithRawJSONSkipped(*sc, c.wantSC) {
253253
t.Fatalf("parseServiceConfig(%s) = %+v, %v, want %+v, %v", c.scjs, sc, err, c.wantSC, c.wantErr)
254254
}
255255
}
@@ -318,7 +318,7 @@ func (s) TestPraseMsgSize(t *testing.T) {
318318

319319
for _, c := range testcases {
320320
sc, err := parseServiceConfig(c.scjs)
321-
if c.wantErr != (err != nil) || !reflect.DeepEqual(sc, c.wantSC) {
321+
if c.wantErr != (err != nil) || !scCompareWithRawJSONSkipped(*sc, c.wantSC) {
322322
t.Fatalf("parseServiceConfig(%s) = %+v, %v, want %+v, %v", c.scjs, sc, err, c.wantSC, c.wantErr)
323323
}
324324
}
@@ -384,3 +384,9 @@ func newDuration(b time.Duration) *time.Duration {
384384
func newString(b string) *string {
385385
return &b
386386
}
387+
388+
func scCompareWithRawJSONSkipped(s1, s2 ServiceConfig) bool {
389+
s1.rawJSONString = nil
390+
s2.rawJSONString = nil
391+
return reflect.DeepEqual(s1, s2)
392+
}

0 commit comments

Comments
 (0)