csds: update xds_client to populate update metadata #4226
Conversation
xds/internal/client/cds_test.go
Outdated
| } | ||
| if diff := cmp.Diff(update, test.wantUpdate, cmpOpts); diff != "" { | ||
| t.Errorf("UnmarshalCluster(%v) = %v want %v", test.resources, update, test.wantUpdate) | ||
| t.Errorf(diff) |
There was a problem hiding this comment.
I think just printing the diff with the (-got, +want) should be good enough.
ec30725 to
1815134
Compare
xds/internal/client/lds_test.go
Outdated
| if (err != nil) != (test.wantErr != "") { | ||
| t.Fatalf("UnmarshalListener(%v) = %v wantErr: %q", test.resources, err, test.wantErr) | ||
| t.Errorf("UnmarshalListener(), got err: %v, wantErr: %v", err, test.wantErr) |
There was a problem hiding this comment.
Why change this to Errorf()?
There was a problem hiding this comment.
Even if the resource is NACK'ed, we still want to check the content in the update and metadata.
- update contains the resource names, so we know what was in the response
- metadata contains the resource version, and errors
There was a problem hiding this comment.
Oh, actually, that's a different thing.
Maybe this should stay Fatal.
There was a problem hiding this comment.
This change wasn't intentional. But it seems to be OK to Error instead of Fatal. I'm going to keep it this way. Let me know if you think otherwise.
There was a problem hiding this comment.
I see that we are using Error instead of Fatal in TestUnmarshalListener_ClientSide as well. But I don't see how useful a diff of the update or the metadata is going to be when the gotErr != wantErr.
There was a problem hiding this comment.
I updated the error check to Fatal. Kept the following checks as Error. It could be useful to compare metadata even if returned update is different.
…date metadata xds_client.go - populate Metadata for the update - When unmarshalling responses, DON'T stop at the first error. Keep processing all the resources, so we know which resources are NACKed. v2/v3 client.go - Send the updates to client_callback.go even when response is NACKed. This is necessary for CSDS's purpose, and also necessary when we later call user's callback with the NACK errors. Test changes
b51a518 to
84b74aa
Compare
all the resources, so we know which resources are NACKed.
necessary for CSDS's purpose, and also necessary when we later call user's
callback with the NACK errors.