Skip to content
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.

Sign up for GitHub

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

Jump to bottom

grpc to support empty response. #1009

Merged
merged 2 commits into from
May 23, 2017
Merged

grpc to support empty response. #1009

merged 2 commits into from
May 23, 2017

Conversation

qiwzhang
Copy link
Contributor

No description provided.

RomanDzhabarov
RomanDzhabarov previously approved these changes May 23, 2017
View reviewed changes
Copy link
Member

@RomanDzhabarov RomanDzhabarov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple of nits, LGTM otherwise

source/common/grpc/rpc_channel_impl.cc Outdated
@@ -54,7 +54,7 @@ void RpcChannelImpl::onSuccess(Http::MessagePtr&& http_response) {

// A gRPC response contains a 5 byte header. Currently we only support unary responses so we
// ignore the header. @see serializeBody().
if (!http_response->body() || !(http_response->body()->length() > 5)) {
if (!http_response->body() || !(http_response->body()->length() >= 5)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: http_response->body()->length() < 5, seems to be more readable

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

test/common/grpc/rpc_channel_impl_test.cc Outdated

Http::MessagePtr response_http_message(new Http::ResponseMessageImpl(
Http::HeaderMapPtr{new Http::TestHeaderMapImpl{{":status", "200"}}}));
helloworld::HelloReply inner_response;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: empty_response might be more clear

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@RomanDzhabarov RomanDzhabarov self-assigned this May 23, 2017
htuch
htuch reviewed May 23, 2017
View reviewed changes
Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @qiwzhang. Have you signed the CLA?

test/common/grpc/rpc_channel_impl_test.cc Outdated
Http::MessagePtr response_http_message(new Http::ResponseMessageImpl(
Http::HeaderMapPtr{new Http::TestHeaderMapImpl{{":status", "200"}}}));
helloworld::HelloReply inner_response;
response_http_message->body() = Common::serializeBody(inner_response);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add an EXPECT on the serialized body size being 5 bytes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

RomanDzhabarov
RomanDzhabarov approved these changes May 23, 2017
View reviewed changes
Copy link
Member

@RomanDzhabarov RomanDzhabarov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@htuch CLA is signed, waiting for your final approval.

@htuch htuch merged commit 34d308e into envoyproxy:master May 23, 2017
@qiwzhang qiwzhang deleted the empty_response branch May 23, 2017 18:15
rshriram pushed a commit to rshriram/envoy that referenced this pull request Oct 30, 2018
@JimmyCYJ
Update OWNERS (envoyproxy#1009)
3aadc6e 
Automatic merge from submit-queue.

Update OWNERS

**What this PR does / why we need it**:

**Which issue this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: fixes #

**Special notes for your reviewer**:

**Release note**:

```release-note
```
jpsim pushed a commit that referenced this pull request Nov 28, 2022
@junr03 @jpsim
coverage: disable until OOM is solved (#1011)
29229cd 
Description: deleting until OOM is fixed.
Risk Level: med - no coverage run. Although it was already broken per #1009

Signed-off-by: Jose Nino <[email protected]>
Signed-off-by: JP Simard <[email protected]>
jpsim pushed a commit that referenced this pull request Nov 28, 2022
@junr03 @jpsim
ci: update to clang 10 and install lcov (#1014)
6ccfa1a 
Description: update clang, install lcov, and bring back coverage for CI.
Risk Level: low
Testing: CI

Fixes #1009 #1012

Signed-off-by: Jose Nino <[email protected]>
Signed-off-by: JP Simard <[email protected]>
jpsim pushed a commit that referenced this pull request Nov 29, 2022
@junr03 @jpsim
coverage: disable until OOM is solved (#1011)
452d230 
Description: deleting until OOM is fixed.
Risk Level: med - no coverage run. Although it was already broken per #1009

Signed-off-by: Jose Nino <[email protected]>
Signed-off-by: JP Simard <[email protected]>
jpsim pushed a commit that referenced this pull request Nov 29, 2022
@junr03 @jpsim
ci: update to clang 10 and install lcov (#1014)
2804980 
Description: update clang, install lcov, and bring back coverage for CI.
Risk Level: low
Testing: CI

Fixes #1009 #1012

Signed-off-by: Jose Nino <[email protected]>
Signed-off-by: JP Simard <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RomanDzhabarov RomanDzhabarov RomanDzhabarov approved these changes

@htuch htuch htuch approved these changes

@lizan lizan lizan approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@qiwzhang @lizan @htuch @RomanDzhabarov