RSDK-2880: Add timestamp information to the replay camera #2371
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,9 +3,11 @@ package testutils | |
|
|
||
| import ( | ||
| "context" | ||
| "sync" | ||
|
|
||
| "go.viam.com/utils/rpc" | ||
| "google.golang.org/grpc" | ||
| "google.golang.org/grpc/metadata" | ||
| ) | ||
|
|
||
| // TrackingDialer tracks dial attempts. | ||
|
|
@@ -48,3 +50,35 @@ func (td *TrackingDialer) DialFunc( | |
| } | ||
| return conn, cached, err | ||
| } | ||
|
|
||
| // ServerTransportStream implements grpc.ServerTransportStream and can be used to test setting | ||
| // metadata in the gRPC response header. | ||
| type ServerTransportStream struct { | ||
| mu sync.Mutex | ||
| grpc.ServerTransportStream | ||
| md metadata.MD | ||
| } | ||
|
|
||
| // NewServerTransportStream creates a new ServerTransportStream. | ||
| func NewServerTransportStream() *ServerTransportStream { | ||
| return &ServerTransportStream{ | ||
| md: metadata.New(make(map[string]string)), | ||
| } | ||
| } | ||
|
|
||
| // SetHeader implements grpc.ServerTransportStream. | ||
|
There was a problem hiding this comment. [nit] might be worth explaining how this is supposed to differ from SendHeader and why in this mock up the code is the same There was a problem hiding this comment. Very good question, I actually meant to follow up on this because I wasn't sure myself, and I forgot. Turns out the different between SetHeader and SendHeader is that SetHeader is supposed to merge the new metadata fields with the existing metadata fields, whereas SendHeader overwrites the metadata and is only supposed to be called once. So I think there were two things that were slightly wrong here: our implementation of SetHeader, which completely overwrites the metadata rather than merging, and the fact that in replaypcd camera I'm calling SendHeader, which afaik will overwrite any metadata already set by SetHeader and error if anyone else tries to call SendHeader or SetHeader after that point. I've changed both; thanks for calling this out! cc @edaniels for awareness of this change (Jeremy's out so can't re-request his review) |
||
| func (s *ServerTransportStream) SetHeader(md metadata.MD) error { | ||
| s.mu.Lock() | ||
| defer s.mu.Unlock() | ||
| for k, v := range md { | ||
| s.md[k] = v | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| // Value returns the value in the metadata map corresponding to a given key. | ||
| func (s *ServerTransportStream) Value(key string) []string { | ||
| s.mu.Lock() | ||
| defer s.mu.Unlock() | ||
| return s.md[key] | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[q] Does the metadata context need to be created every time? If not we could move lines 443-444 to 435 and call:
ctx = grpc.NewContextWithServerTransportStream(context.Background(), serverStream)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't need to be, but I felt this more closely mimicked how this works in prod with consecutive gRPC calls using separate ServerTransportStreams and gRPC contexts. So I'm going to leave this as-is unless someone else feels strongly that this should change.