-
Notifications
You must be signed in to change notification settings - Fork 15
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Return nil from Server on stream shutdown #24
Return nil from Server on stream shutdown #24
Conversation
8a8ba7a
to
a4ca19f
Compare
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.
LGTM
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.
💯
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.
I like the last changes around arrowpb.StatusCode_STREAM_SHUTDOWN
LGTM
After merging #24, unit tests for otelarrowreceiver were failing and I did not catch that. This PR fixes the unit tests by adding in support for an extra call to Send() the receiver will do (with arrowpb.StatusCode_STREAM_SHUTDOWN), to signal shutdown to the client. Addresses #29 (comment)
Release v0.3.0 includes: - #24 - #29 - #30 - #36 This release includes a BREAKING CHANGE 🛑 🛑 🛑 🛑 🛑. We do not anticipate another of these, it merely renames/enumbers status codes to match the gRPC status code numbering scheme. However, since status code 0 stays the same, it will result in unusual looking errors but should be safe to deploy the receivers first. Their CANCELED (i.e., SERVER_SHUTDOWN) will read as UNAVAILABLE, etc. See #27.
Fixes #6
Applying learning from grpc/grpc-go#6504 (comment), which pointed out that the server is return EOF directly when the client calls CloseSend(), which is causing an error signal on spans.
Instead the server should check if it received EOF from client (indicating CloseSend() was called) and send StatusOK to the client. The client will know to restart the stream when it gets a response with
batchID=-1
andstatus=OK
.