Parse ServiceConfig JSON string #1515

lyuxuan · 2017-09-14T01:49:10Z

No description provided.

menghanl · 2017-09-29T00:17:42Z

clientconn.go

+	Method  *string `json:"method,omitempty"`
+}
+
+func (j jsonName) Stringer() string {


s/Stringer()/String() ?
Stringer is the name of an interface with a String() function.

menghanl · 2017-10-06T18:22:45Z

clientconn.go

+		res += "/" + *j.Service + "/"
+	}
+	if j.Method != nil {
+		res += *j.Method


If service == nil, this function returns "method", is this right?

When searching in the map, we search for /service/method first, then /service/, but never for method. Do we need to store the value in the map if service == nil?

right. change has been made to skip storing such method config.

menghanl · 2017-10-06T18:27:47Z

clientconn.go

+		return err
+	}
+	cc.scRaw = js
+	var sc ServiceConfig


sc := ServiceConfig { LB: rsc.LoadBalancingPolicy, Methods: make(map[string]MethodConfig), }

menghanl · 2017-10-06T18:36:14Z

clientconn.go

+
+// handleServiceConfig parses the service config string in JSON format to Go native
+// struct ServiceConfig, and store both the struct and the JSON string in ClientConn.
+func (cc *ClientConn) handleServiceConfig(js string) error {


Move the all service config related code (structs and functions) to a new file service_config.go and also add unit tests for the json parsing function.

Put all the parsing code in a function

func parseJsonServiceConfig(js string) (ServiceConfig, error)

In ClientConn, you can do

if sc, err := parse(js); err == nil { cc.mu.Lock() cc.scRaw = js cc.sc = sc cc.mu.Unlock() }

menghanl · 2017-10-06T18:43:52Z

test/end2end_test.go

@@ -1226,104 +1230,192 @@ func newDuration(b time.Duration) (a *time.Duration) {
 func TestServiceConfigGetMethodConfig(t *testing.T) {
 	defer leakcheck.Check(t)
 	for _, e := range listTestEnv() {
-		testGetMethodConfig(t, e)
+		if e.balancer == "roundrobin" {


It should be OK to test service config with only one of the test envs. Then we don't need this wrapper function and loop.

Just start the test with

te := testServiceConfigSetup(t, tcpClearRREnv)

menghanl · 2017-10-06T18:45:11Z

test/end2end_test.go

-		Methods: m,
-	}
-	ch <- sc
+	scjs := `{


Move the code around so the declaration of a variable is closed to where it's used?

You actually don't need this string variable if you don't need to check against it.

menghanl · 2017-10-06T18:47:11Z

test/end2end_test.go

+	// Make sure service config has been processed by grpc.
+	mc := cc.GetMethodConfig("/grpc.testing.TestService/EmptyCall")
+	for {
+		if mc.WaitForReady == nil {


for { if cc.GetMethodConfig("/grpc.testing.TestService/EmptyCall").WaitForReady != nil { break } time.Sleep(time.Millisecond) }

Probably also add some timeout:

var processed bool for i := 0; i < 1000; i++ { if cc.GetMethodConfig("/grpc.testing.TestService/EmptyCall").WaitForReady != nil { processed = true break } time.Sleep(time.Millisecond) } if !processed { t.Fatalf("service config is not processed by gRPC after 1 second") }

this code logic has been deleted in recent commit.

Did you forget to push some commits?
This code is still here.

menghanl · 2017-10-12T19:06:24Z

clientconn.go

+// struct ServiceConfig, and store both the struct and the JSON string in ClientConn.
+func (cc *ClientConn) handleServiceConfig(js string) error {
+	sc, err := parseServiceConfig(js)
+	if err == nil {


Return error early instead.

sc, err := parseServiceConfig(js) if err != nil { return err } cc.mu.Lock() cc.scRaw = js cc.sc = sc cc.mu.Unlock() }

menghanl · 2017-10-12T19:07:15Z

rpc_util.go

@@ -524,7 +524,7 @@ type MethodConfig struct {
 type ServiceConfig struct {


Move this struct to the new file, too?

menghanl · 2017-10-12T19:10:11Z

service_config.go

+	Method  *string `json:"method,omitempty"`
+}
+
+func (j jsonName) String() (string, bool) {


Typically, String() function only returns string.
Rename this to getPath().

menghanl · 2017-10-12T19:16:09Z

service_config_test.go

+    			"name": [
+    				{
+    					"service": "foo",
+              "method": "Bar"


Indentation for this and some of the following tests.

Still doesn't look right...

menghanl · 2017-10-12T19:17:22Z

service_config_test.go

+)
+
+func TestParseLoadBalancer(t *testing.T) {
+	defer leakcheck.Check(t)


Leakcheck is unnecessary for tests like this.

menghanl · 2017-10-13T17:55:23Z

test/end2end_test.go

+	// Make sure service config has been processed by grpc.
+	mc := cc.GetMethodConfig("/grpc.testing.TestService/EmptyCall")
+	for {
+		if mc.WaitForReady == nil {


Did you forget to push some commits?
This code is still here.

menghanl · 2017-10-13T20:23:30Z

service_config.go

+	MaxResponseMessageBytes *int        `json:"maxResponseMessageBytes,omitempty"`
+}
+
+type jsonSC struct {


This struct is same as the real ServiceConfig struct except for the second field. I prefer to not have this temporary struct, and reuse the existing structs.

json does a partial parsing if the struct only contains part of the fields, so we could do Unmarshal twice, first time for the service/method name, second time for the remaining configs. The second Unmarshal can go directly to the existing MethodConfig struct.

https://play.golang.org/p/GCSXcP1yQk

const testJSON = `{"name": [ {"service": "foo", "method": "bar"} ], "waitForReady": true}` type jsonName struct { Service *string `json:"service,omitempty"` Method *string `json:"method,omitempty"` } type methodConfigName struct { Name *[]jsonName } type MethodConfig struct { WaitForReady *bool Timeout *string } func main() { buf := []byte(testJSON) var mcn methodConfigName err := json.Unmarshal(buf, &mcn) // for name only. if err != nil { panic(err.Error()) } fmt.Printf("%+v\n", mcn) var mc MethodConfig err = json.Unmarshal(buf, &mc) // for everything other than name. if err != nil { panic(err.Error()) } fmt.Printf("%+v\n", mc) }

filed names are different and go json package cannot handle it.

menghanl · 2017-10-18T19:30:16Z

clientconn.go

@@ -577,6 +579,7 @@ type ClientConn struct {

 	mu    sync.RWMutex
 	sc    ServiceConfig
+	scRaw string


Set this to "" in scWatcher? Just to be "consistent".

menghanl · 2017-10-18T19:48:48Z

service_config_test.go

+    			"name": [
+    				{
+    					"service": "foo",
+              "method": "Bar"


Still doesn't look right...

menghanl

LGTM

… json

lyuxuan requested a review from menghanl September 14, 2017 17:52

menghanl self-assigned this Sep 14, 2017

dfawley assigned lyuxuan and unassigned menghanl Sep 21, 2017

lyuxuan force-pushed the handleSC branch from 06b82f7 to 66c56b6 Compare September 22, 2017 02:58

lyuxuan closed this Sep 22, 2017

lyuxuan reopened this Sep 22, 2017

dfawley assigned menghanl and unassigned lyuxuan Sep 28, 2017

lyuxuan force-pushed the handleSC branch 2 times, most recently from f539b14 to 04aa293 Compare October 5, 2017 22:51

menghanl added the Type: Feature New features or improvements in behavior label Oct 6, 2017

menghanl reviewed Oct 6, 2017

View reviewed changes

menghanl reviewed Oct 13, 2017

View reviewed changes

lyuxuan force-pushed the handleSC branch from 8627b19 to ef70aa1 Compare October 13, 2017 23:32

menghanl mentioned this pull request Oct 16, 2017

Is balancer api v2 stable? #1579

Closed

menghanl reviewed Oct 18, 2017

View reviewed changes

menghanl approved these changes Oct 18, 2017

View reviewed changes

lyuxuan added 11 commits October 19, 2017 11:40

initial commit

265af2d

unexport service config struct in end2end_test.go

504c356

revert old service config logic back

c8531fa

unexport handleServiceConfig and fix tests

2c737d8

uncomment nonBlockingDial

c410f25

resolve race

29021ff

seprate service config logic into a single file, added test for parse…

b594598

… json

added test for parsing load_balancer

5530346

resolve go1.7 error string discrepancy

63c9f26

fix json string indentation, minor fixes

9240c1a

proto3

f2931ef

lyuxuan added 10 commits October 19, 2017 11:47

remove unnecessary leakcheck

42ccf54

remove more unnecessary leak check

a5846ba

colocate service config functions

361f0f2

deprecate old service config impl

3920b95

deprecated

52f94b9

minor

56bd50d

remove unnecessary json tags

c703add

fix json string indentation

c7a5628

in old service config path also set scRaw to empty

a5da4b2

fix minor

19f9209

lyuxuan force-pushed the handleSC branch from c6cd7a9 to 19f9209 Compare October 19, 2017 18:52

lyuxuan merged commit 6f3b6ff into grpc:master Oct 19, 2017

menghanl added this to the 1.8 Release milestone Nov 7, 2017

lock bot locked as resolved and limited conversation to collaborators Jan 18, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parse ServiceConfig JSON string #1515

Parse ServiceConfig JSON string #1515

lyuxuan commented Sep 14, 2017

menghanl Sep 29, 2017

lyuxuan Oct 11, 2017

menghanl Oct 6, 2017

lyuxuan Oct 11, 2017

menghanl Oct 6, 2017

lyuxuan Oct 11, 2017

menghanl Oct 6, 2017

lyuxuan Oct 11, 2017

menghanl Oct 6, 2017

lyuxuan Oct 11, 2017

menghanl Oct 6, 2017

lyuxuan Oct 11, 2017

menghanl Oct 6, 2017

lyuxuan Oct 11, 2017

menghanl Oct 13, 2017

lyuxuan Oct 13, 2017

menghanl Oct 12, 2017

lyuxuan Oct 13, 2017

menghanl Oct 12, 2017

lyuxuan Oct 13, 2017

menghanl Oct 12, 2017

lyuxuan Oct 13, 2017

menghanl Oct 12, 2017

lyuxuan Oct 13, 2017

menghanl Oct 18, 2017

menghanl Oct 12, 2017

lyuxuan Oct 13, 2017 •

edited

Loading

menghanl Oct 13, 2017

menghanl Oct 13, 2017

lyuxuan Oct 13, 2017

menghanl Oct 18, 2017

menghanl Oct 18, 2017

menghanl left a comment

		@@ -524,7 +524,7 @@ type MethodConfig struct {
		type ServiceConfig struct {

Parse ServiceConfig JSON string #1515

Parse ServiceConfig JSON string #1515

Conversation

lyuxuan commented Sep 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lyuxuan Oct 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

menghanl left a comment

Choose a reason for hiding this comment

lyuxuan Oct 13, 2017 •

edited

Loading