send raw table update to clients #14

vtolstov · 2019-07-28T23:00:54Z

Signed-off-by: Vasiliy Tolstov [email protected]

vtolstov · 2019-07-28T23:02:40Z

Don't over allocate memory for json Marshal/Unmarshal, but fill needed structs directly via reflect.
Also send native row structs in updates, so go-ovn does not need to convert it back, but use only type assertion.
Pr to go-ovn repo prepeared (now only convert logical switch operations)

vtolstov · 2019-07-28T23:04:09Z

later we can generate row_types.go file from ovnnb schema files via go generate

vtolstov · 2019-07-30T16:03:56Z

now it have ugly things to parse empty sets and bools. why in Connection table max_backoff passed via set ?

hzhou8 · 2019-07-30T21:21:50Z

Thanks for working on this! I have some major comments:

We should not introduce any OVN specific code in libovsdb. In rawToRowUpdates(), it should be generic enough. All OVN specific code should be in go-ovn.
We may consider auto generate the code for any given OVSDB schema. The code generation tool should be in libovsdb. In go-ovn it can use this tool to generate code for processing.

What do you think?

(it's ok to continue your test this way as POC)

vtolstov · 2019-07-30T21:28:15Z

@hzhou8

this changes not only ovn related. This changes ovsdb related. So i think that this is proper place. For example i create SSL Connection and other structs to libovsdb. So when we create client for openvswitch db, we can work with native go structs and not maps
code generator for libovsdb is fine it can generate needed structs for all message updates. But why it needed for go-ovn?

vtolstov · 2019-07-31T12:32:07Z

@hzhou8 i'm finish pr.
i think that now it completed, also i'm push to go-ovn repo some files that already uses this pr

hzhou8 · 2019-07-31T18:44:48Z

Thanks @vtolstov ! I need to review it in more detail, but here are just some minor things:

Could you add more detailed description in the commit message?
The SSL/TCP const rename is not related to this change, so please split to another PR.
There are still "ovn" in comments. It is better to be removed.

vtolstov · 2019-07-31T19:38:14Z

@hzhou8 thanks, if fix suggested

rpc_test.go

hzhou8 · 2019-08-02T02:21:15Z

client.go

@@ -49,12 +48,12 @@ var (
 const (
 	defaultTCPAddress  = "127.0.0.1:6640"
 	defaultUnixAddress = "/var/run/openvswitch/ovnnb_db.sock"
-	SSL             = "ssl"


Still unrelated changes :)

convert.go

dont try to convert between types via json marshal/unmarshal send rpc payload directly as table updates allow to consumers convert it to native structs via provided functions MapToStruct and StructToMap Signed-off-by: Vasiliy Tolstov <[email protected]>

vtolstov · 2019-08-08T18:43:50Z

@hzhou8 any chance to get this merged? or i need to fix something more?

hzhou8 · 2019-08-08T19:19:13Z

@vtolstov sorry that I was distracted last week, but I did spent some time reviewing it, and I am still not very clear about the purpose of this change. Is it purely for performance optimization (i.e. to avoid marshal/unmarshal) or is it also an interface change (i.e. the format changed of tableUpdates generated by update())? If it is for performance, then is there test result telling how much did it improve? It did add a lot more complexity and if the performance gain is small I would be hesitating. I didn't see enough tests to tell the purpose of the change. I probably need more time to write some tests myself to understand more about it. I am also requesting adding more tests for such big change to make sure there is no regression problem.

vtolstov · 2019-08-08T19:31:45Z

Thanks, this is very small change for sending updates, most of the changes needed for go-ovn to get native structs from map.
Also this adds some basic validation of format of row updates that must be like in rfc. Older version of test sent invalid row update.
Format of row update not changed, mostly this changes to avoid marshal/unmarshal. And fills UUID

vtolstov · 2019-08-08T19:32:06Z

So this is compatible change that not change for consumers anything.

vtolstov · 2019-08-08T19:33:34Z

Mostly this is not have major speedup, but minimize unneeded allocations.

hzhou8

Hi @vtolstov, sorry for the slow response. I did some tests myself to understand the intent of the change. Please see my comments/questions inlined. In addition, here are some more general comments:

Please update the example/play_with_ovs.go, which is now broken by this change.
The module convert.go seems not used in this repo, so I guess it is supposed to be used as an API by other project, e.g. go-ovn, right? If so, please add basic test cases to test the interfaces and also as example to demonstrate how to use them.
If we want to treat this as a prototype and if adding documentation and test cases seem to be too heavy for the prototype phase, then I'd suggest to merge it to a branch instead of master. When the prototype is proved mature and all the documentation and tests are ready, we can merge it to master.

hzhou8 · 2019-08-26T23:23:21Z

rpc_test.go

-	validRowUpdate := make(map[string]RowUpdate)
-	validRowUpdate["uuid"] = RowUpdate{}
-	validUpdate["table"] = validRowUpdate
+	// Valid dummy update should pass https://tools.ietf.org/html/rfc7047#section-4.1.6


Please update the comments, since you are now changing the test from testing a dummy update to testing a non-dummy update, i.e. with a new row insertion.

hzhou8 · 2019-08-26T23:23:42Z

rpc_test.go

-	validUpdate["table"] = validRowUpdate
+	// Valid dummy update should pass https://tools.ietf.org/html/rfc7047#section-4.1.6
+	updates := make(map[string]interface{})
+	updates["Test_Table"] = make(map[string]interface{})


This is not needed.

hzhou8 · 2019-08-26T23:23:59Z

rpc_test.go


-	err = update(nil, []interface{}{"hello", validUpdate}, &reply)
+	tableUpdate := make(map[string]interface{})
+	tableUpdate["Test_UUID"] = make(map[string]interface{})


hzhou8 · 2019-08-26T23:24:28Z

rpc_test.go

+	tableUpdate["Test_UUID"] = make(map[string]interface{})
+
+	newUpdate := make(map[string]interface{})
+	newUpdate["new"] = make(map[string]interface{})


hzhou8 · 2019-08-26T23:56:32Z

client.go

+func rawToRowUpdates(raw map[string]interface{}) (map[string]map[string]RowUpdate, error) {
+	rowUpdates := make(map[string]map[string]RowUpdate)
+
+	for tbl, opt := range raw {


Why the variable named "opt" here? Is it more straightforward to be: "tblUpdate"?

hzhou8 · 2019-08-27T00:03:02Z

client.go

+			continue
+		}
+		rowUpdates[tbl] = make(map[string]RowUpdate)
+		for uuid, val := range opts {


Suggest to rename "val" to "rowUpdate"

hzhou8 · 2019-08-27T00:05:20Z

client.go

+	for tbl, opt := range raw {
+		opts, ok := opt.(map[string]interface{})
+		if !ok {
+			return nil, fmt.Errorf("invalid row update: %v", opt)


Error message can be enhanced to "invalid TableUpdate"

hzhou8 · 2019-08-27T00:06:40Z

client.go

+	rowUpdates := make(map[string]map[string]RowUpdate)
+
+	for tbl, opt := range raw {
+		opts, ok := opt.(map[string]interface{})


opts => rowUpdates

hzhou8 · 2019-08-27T00:11:21Z

client.go

+			for k, vv := range vals {
+				v, ok := vv.(map[string]interface{})
+				if !ok {
+					return nil, fmt.Errorf("invalid row update: %v", vv)


It is better to print "old" or "new" in the error message to help debugging

hzhou8 · 2019-08-27T04:33:14Z

client.go

@@ -182,25 +181,58 @@ func echo(client *rpc2.Client, args []interface{}, reply *[]interface{}) error {
 	return nil
 }

+func rawToRowUpdates(raw map[string]interface{}) (map[string]map[string]RowUpdate, error) {


Originally, the rowUpdate uses Row struct, which has OvsMap, OvsSet, etc. embedded. Now this patch use a slice to represent the value of each column. For example, originally a column "external_ids" is like:

"external_ids":libovsdb.OvsMap{GoMap:map[interface {}]interface {}{"docker":"made-for-each-other", "go":"awesome"}}

Now it is changed to:
"external_ids":[]interface {}{"map", []interface {}{[]interface {}{"docker", "made-for-each-other"}, []interface {}{"go", "awesome"}}}

The first element in the slice is the string "map", probably representing the data type, and the second element is a slice of interface{}, each element representing a key-value pair in the "external_ids" column. Each key-value pair is again represented by a slice of interface{} which has two element: the first element is the key, and the second element is the value.

Is this slices of slices of slices the desired output of this function? How is it more "native" than the original implementation? Could you explain a little more? I think it is better to have some formal documentation to describe the design. I'd also insist to have some basic test cases to verify if the function behaves as expected, i.e. given some typical input of "raw", make sure the return value is expected as per the design.

vtolstov · 2021-02-03T10:44:26Z

close in favour of #28

vtolstov requested a review from hzhou8 July 28, 2019 23:03

vtolstov changed the title ~~[WIP] send native structs in table updates~~ send raw table update to clients Jul 31, 2019

hzhou8 suggested changes Aug 2, 2019

View reviewed changes

send raw table update to clients

8bae5b9

dont try to convert between types via json marshal/unmarshal send rpc payload directly as table updates allow to consumers convert it to native structs via provided functions MapToStruct and StructToMap Signed-off-by: Vasiliy Tolstov <[email protected]>

vtolstov requested a review from hzhou8 August 8, 2019 18:43

hzhou8 suggested changes Aug 27, 2019

View reviewed changes

vtolstov closed this Feb 3, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

send raw table update to clients #14

send raw table update to clients #14

vtolstov commented Jul 28, 2019

vtolstov commented Jul 28, 2019

vtolstov commented Jul 28, 2019

vtolstov commented Jul 30, 2019

hzhou8 commented Jul 30, 2019

vtolstov commented Jul 30, 2019

vtolstov commented Jul 31, 2019 •

edited

Loading

hzhou8 commented Jul 31, 2019

vtolstov commented Jul 31, 2019

hzhou8 Aug 2, 2019

vtolstov commented Aug 8, 2019

hzhou8 commented Aug 8, 2019

vtolstov commented Aug 8, 2019

vtolstov commented Aug 8, 2019

vtolstov commented Aug 8, 2019

hzhou8 left a comment

hzhou8 Aug 26, 2019

hzhou8 Aug 26, 2019

hzhou8 Aug 26, 2019

hzhou8 Aug 26, 2019

hzhou8 Aug 26, 2019

hzhou8 Aug 27, 2019

hzhou8 Aug 27, 2019

hzhou8 Aug 27, 2019

hzhou8 Aug 27, 2019

hzhou8 Aug 27, 2019

vtolstov commented Feb 3, 2021

send raw table update to clients #14

send raw table update to clients #14

Conversation

vtolstov commented Jul 28, 2019

vtolstov commented Jul 28, 2019

vtolstov commented Jul 28, 2019

vtolstov commented Jul 30, 2019

hzhou8 commented Jul 30, 2019

vtolstov commented Jul 30, 2019

vtolstov commented Jul 31, 2019 • edited Loading

hzhou8 commented Jul 31, 2019

vtolstov commented Jul 31, 2019

Choose a reason for hiding this comment

vtolstov commented Aug 8, 2019

hzhou8 commented Aug 8, 2019

vtolstov commented Aug 8, 2019

vtolstov commented Aug 8, 2019

vtolstov commented Aug 8, 2019

hzhou8 left a 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

vtolstov commented Feb 3, 2021

vtolstov commented Jul 31, 2019 •

edited

Loading