Refactor stream connections for better readability, add stream metrics#131
Refactor stream connections for better readability, add stream metrics#131temporal-nick merged 4 commits intomainfrom
Conversation
temporal-nick
changed the title
| type StreamRequestOrResponse interface { | ||
| adminservice.StreamWorkflowReplicationMessagesRequest | adminservice.StreamWorkflowReplicationMessagesResponse | ||
| } | ||
| type ValueWithError[T StreamRequestOrResponse] struct { | ||
| val *T | ||
| err error | ||
| } | ||
| type recvable[T StreamRequestOrResponse] interface { | ||
| Recv() (*T, error) | ||
| } |
There was a problem hiding this comment.
nit: our style is to use a single type block for adjacent type declartions
(but it passes lint so 🤷 )
There was a problem hiding this comment.
Will update in a new PR
| } | ||
| }() | ||
|
|
||
| wg.Add(2) |
There was a problem hiding this comment.
Before, it looks like we didn't wait for both goroutines to finish. We only waited for one of the two 🤔
Seems like a good idea to wait for them both to finish, but calling it out as a difference anyway.
There was a problem hiding this comment.
Oh yeah, there's a little more machinery here now: I have the two goroutines communicate on when to stop so that both sides hangup when one dies. The major difference here is we will be returning io.EOF and hanging up the client more reliably in more situations. In non-exceptional behavior, it's the same.
| return targetStreamServerData | ||
| } | ||
|
|
||
| func transferSourceToTarget( |
There was a problem hiding this comment.
nit: it looks like the same parameter list for the two transfer funcs (transferSourceToTarget and transferTargetToSource). Can we make a new type that has those two funcs as methods?
One maybe benefit is that would enable shared state as an option instead of message passing, if that seems useful or simpler here (I haven't thought about it too closely though)
There was a problem hiding this comment.
I did try pretty hard to combine these, but I couldn't settle on a sane implementation. We'd need a pretty gnarly interface to grab what we needed out of the GRPC types, because the request and response aren't symmetric that way
temporalio#131) * Refactor stream connections for better readability * remove breaks to conform to linter * Native histograms are behaving badly in Grafana, remove for now * Remove remaining native histogram
2 participants
What was changed
Added a bunch of metrics:
Also refactored the logic in StreamWorkflowMessages to make it a little more obviously symmetric.