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.

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

remove exception to status legacy code from http codec #15773

Merged
merged 5 commits into from
Mar 31, 2021

Conversation

chaoqin-li1123
Copy link
Member

@chaoqin-li1123 chaoqin-li1123 commented Mar 30, 2021

Signed-off-by: chaoqin-li1123 chaoqinli@google.com

For an explanation of how to fill out the fields, please see the relevant section
in PULL_REQUESTS.md

Commit Message: exceptionToStatus is used in the migration from C++ exception to error status in http codec. since the exception is no longer used, remove the exception and the exceptionToStatus utility function from the codebase. This is also part of the effort to remove exception from data plane. (#14320)
Additional Description:
Risk Level:low
Testing:none
Docs Changes:none
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]
[Optional API Considerations:]

chaoqin-li1123 added 2 commits March 30, 2021 16:05
Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>
Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>
asraa
asraa previously approved these changes Mar 30, 2021
Copy link
Contributor

@asraa asraa left a comment

Choose a reason for hiding this comment

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

Awesome! Thank you for the cleanup.

BTW Got worried in case there were random other EnvoyExceptions floating through some helper methods in the dispatch callstack, but just saw they would have been uncaught and we would have seen them.

Thank you!

@chaoqin-li1123
Copy link
Member Author

/retest

@repokitteh-read-only
Copy link

Retrying Azure Pipelines:
Check envoy-presubmit isn't fully completed, but will still attempt retrying.
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #15773 (comment) was created by @chaoqin-li1123.

see: more, trace.

Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Awesome, thanks. Can we remove innerDispatch() also as I don't think it's needed anymore?

/wait

@chaoqin-li1123
Copy link
Member Author

Awesome, thanks. Can we remove innerDispatch() also as I don't think it's needed anymore?

/wait

Sure, that sounds reasonable.

chaoqin-li1123 added 3 commits March 31, 2021 04:35
Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>
Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>
Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>
@asraa
Copy link
Contributor

asraa commented Mar 31, 2021

Thanks for catching that Matt!

Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Awesome!

@mattklein123 mattklein123 merged commit 4d5c795 into envoyproxy:main Mar 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants