-
Notifications
You must be signed in to change notification settings - Fork 47
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
stream: Fix default data won't be received issue #208
stream: Fix default data won't be received issue #208
Conversation
Codecov ReportPatch coverage has no change and project coverage change:
Additional details and impacted files@@ Coverage Diff @@
## master #208 +/- ##
==========================================
- Coverage 24.42% 24.35% -0.07%
==========================================
Files 16 16
Lines 2645 2652 +7
==========================================
Hits 646 646
- Misses 1999 2006 +7
☔ View full report in Codecov by Sentry. |
would it be worth adding a message to the example that validates this? Sounds like it is pretty easy to replicate by passing 0's here ttrpc-rust/example/async-stream-client.rs Lines 124 to 125 in 555c412
|
And verify containerd#208 Signed-off-by: Tim Zhang <[email protected]>
@jsturtevant Good suggestion, I've just finished it and please help to review it again, thanks |
Protobuf encodes default values, e.g., 0 for number, "" for string, as empty payload and we wouldn't send empty payload to a receiver. This commit change the method of empty message detecting. Fixes containerd#169 Fixes containerd#207 Signed-off-by: Tim Zhang <[email protected]>
Add test for issues: containerd#169, containerd#207 Signed-off-by: Tim Zhang <[email protected]>
5884e9a
to
2678850
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.
Thanks, 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.
LGTM but only one small nit
example/async-stream-client.rs
Outdated
@@ -120,12 +120,17 @@ async fn sum_stream(cli: streaming_ttrpc::StreamingClient) { | |||
|
|||
#[cfg(unix)] | |||
async fn divide_stream(cli: streaming_ttrpc::StreamingClient) { | |||
let mut stream = cli |
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.
nit: Could we create a test case to test the default value specifically instead of messing it into divide_stream
?
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.
A new case was added instead of modifying one of the exists.
Add test for issues: containerd#169, containerd#207. Signed-off-by: Tim Zhang <[email protected]>
2678850
to
e16aa7b
Compare
Cut the release for containerd#196, containerd#197, containerd#200, containerd#203, containerd#208 Signed-off-by: Tim Zhang <[email protected]>
Protobuf encodes default values, e.g., 0 for number, "" for string, as empty payload and we wouldn't send empty payload to a receiver.
This commit change the method of empty message detecting.
Fixes #169
Fixes #207