-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
grpc: Refactor decode method #32676
grpc: Refactor decode method #32676
Conversation
Signed-off-by: tyxia <tyxia@google.com>
Signed-off-by: tyxia <tyxia@google.com>
Signed-off-by: tyxia <tyxia@google.com>
Signed-off-by: tyxia <tyxia@google.com>
…codec_client Signed-off-by: tyxia <tyxia@google.com>
/assign @yanavlasov @htuch |
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.
Looks like a good approach. Please make in non-draft when it is ready for review
Thank for review! This PR is ready for review. (I just temporarily marked it as draft to prioritize my other outstanding PRs, as this pr is for status code improvement) I will reopen this PR once the other gRPC change is merged |
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.
/wait-any
Signed-off-by: tyxia <tyxia@google.com>
/retest |
/wait Add release note before submission |
/retest |
@@ -300,7 +300,7 @@ Http::FilterDataStatus GrpcWebFilter::encodeData(Buffer::Instance& data, bool en | |||
// The decoder always consumes and drains the given buffer. Incomplete data frame is buffered | |||
// inside the decoder. | |||
std::vector<Grpc::Frame> frames; | |||
decoder_.decode(data, frames); | |||
std::ignore = decoder_.decode(data, frames); |
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.
Should these turn into TODOs or bugs?
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.
Waiting for final review from @htuch /wait-any |
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, thanks!
This PR is to improve the error status/code In PR envoyproxy#32511, we introduce a max_frame_length feature (optional) . Now gRPC frame decoding can fail EITHER (1) due to decoding error OR (2) due to over-frame-limit error. To better surface the error message, this PR refactor return type from bool to absl::status , so that the caller site can differentiate the error status. source/common/grpc/async_client_impl.cc in this PR can be an user example Risk level: Low Testing: Unit tests Signed-off-by: tyxia <tyxia@google.com>
This PR is to improve the error status/code
In PR #32511, we introduce a
max_frame_length
feature (optional) . Now gRPC frame decoding can fail EITHER (1) due to decoding error OR (2) due to over-frame-limit error.To better surface the error message, this PR refactor return type from
bool
toabsl::status
, so that the caller site can differentiate the error status.source/common/grpc/async_client_impl.cc
in this PR can be an user example