fixes #issue 42. add num for each key and sort keys by num. make sure keys in map can be sorted. #50
Conversation
| @@ -170,7 +172,10 @@ | ||
| // Recover escaped '[' and ']'. | ||
| val = strings.Replace(val, `\]`, `]`, -1) | ||
| val = strings.Replace(val, `\[`, `[`, -1) |
There was a problem hiding this comment.
Good find!
I meant the patch that orders the keys as present in path.
transl_utils/transl_utils.go
Outdated
| log.V(6).Infof("num_key_map : %#v", num_key_map) | ||
| log.V(6).Infof("key_val_map : %#v", key_val_map) | ||
|
|
||
| if num_key_map != nil { |
There was a problem hiding this comment.
Always return != nil, as you have made it with make. You can check len instead.
transl_utils/transl_utils.go
Outdated
| @@ -94,8 +98,32 @@ func ConvertToURI(prefix *gnmipb.Path, path *gnmipb.Path, req *string) error { | |||
| /* If keys are present , process the keys. */ | |||
| if key != nil { | |||
| for k, v := range key { | |||
| log.V(6).Infof("elem : %#v %#v", k, v) | |||
| *req += "[" + k + "=" + v + "]" | |||
| //log.V(6).Infof("elem : %#v %#v", k, v) | |||
There was a problem hiding this comment.
Please remove commented lines.
transl_utils/transl_utils.go
Outdated
| log.V(6).Infof("num : %#v", num_str) | ||
| key_del_num := k[strings.Index(k, "]") + 1 : len(k)] | ||
| log.V(6).Infof("elem : %#v %#v", key_del_num, v) | ||
| num, err := strconv.Atoi(num_str) |
There was a problem hiding this comment.
You may remove these 2 log lines. Later log at 115 & 116 should be sufficient.
| for num_key_map_k := range num_key_map { | ||
| num_list = append(num_list, num_key_map_k) | ||
| } | ||
| sort.Ints(num_list) |
There was a problem hiding this comment.
Shouldn't this be already sorted?
xPath patch uses it in sequence.
|
@renukamanavalan I fixed by suggestion fix, and add a cace when key has no tag for test module. |
|
@ganglyu & @zbud-msft - Please provide your review/approval. |
| // "[k1=v1][k2=v2]", | ||
| // this API returns | ||
| -// map[string]string{"k1": "v1", "k2": "v2"}. | ||
| +// map[string]string{"[0]k1": "v1", "[1]k2": "v2"}. |
There was a problem hiding this comment.
How about compatibility?
If we add num for each key, other GNMI client can't communicate with SONiC telemetry?
There was a problem hiding this comment.
The scope of this numbering within parameters of a path element.
@pettershao-ragilenetworks -- please correct my statement if needed.
|
this bug cause by GNMI client, the client assign value to map with "keyValuePairs[key] = val", causes the k-vs to be out of order in memory. I used the int(-9223372036854775808 ~ 9223372036854775807) type in c-s, but there are rarely more than 10 keys in practice. If key without tag, the request also can be accepted, but the order of map to path cannot be guaranteed, and the resource may not be found. see file transl_utils/transl_utils.go in line 101. |
I wonder if GNMI spec has any solution for this order issue. |
|
https://github.com/openconfig/gnmi/blob/master/proto/gnmi/gnmi.proto#L153-L156 |
|
I had try use list[map,map] struct replace map or add a list to record key in protobuf(modify protobuf defined message), but failed. Pkg YGOT relies on the structure generated by protobuf, cause can not use YGOT, so at last I gived up, and the code is more than now. @ganglyu |
|
@seiferteric Would you please review this PR? |
Why I did it
The path is not fixed due to the unordered property of the map，which may cause the resource to be unable to be found.
#42
How I did it
add num for each key and sort keys by num.
How to verify it
gnmi_get,gnmi_set's path has more than two keys.
Which release branch to backport (provide reason below if selected)
Description for the changelog
Link to config_db schema for YANG module changes
A picture of a cute animal (not mandatory but encouraged)