-
Notifications
You must be signed in to change notification settings - Fork 5.2k
dynamic_module: add new streamable HTTP callout support #42225
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
Conversation
b498e2d to
f9ea7e9
Compare
wbpcode
left a comment
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 so much for picking this up. One comment to the ABI to start the review. :)
1f1f7d3 to
bc82115
Compare
Signed-off-by: Rohit Agrawal <[email protected]>
9126681 to
47e213a
Compare
Signed-off-by: Rohit Agrawal <[email protected]>
47e213a to
a81c6f8
Compare
|
CC @envoyproxy/coverage-shephards: FYI only for changes made to |
mathetake
left a comment
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.
This is so cool, thank you so much for taking this up! I am trying to think about the way to remove the necessity for "unsafe" on these pointer passing methods but yeah i think there's no way around it seems..
Signed-off-by: Rohit Agrawal <[email protected]>
Signed-off-by: Rohit Agrawal <[email protected]>
Signed-off-by: Rohit Agrawal <[email protected]>
Signed-off-by: Rohit Agrawal <[email protected]>
wbpcode
left a comment
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 so much for this great contribution. The new ABI lgtm to me. And some comments to the implementation code. :)
/wait
Signed-off-by: Rohit Agrawal <[email protected]>
| source/extensions/filters/common/lua: 95.6 | ||
| source/extensions/filters/http/cache: 95.9 | ||
| source/extensions/filters/http/dynamic_forward_proxy: 94.8 | ||
| source/extensions/filters/http/dynamic_modules: 95.6 |
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.
Could we add additional tests to increase coverage?
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.
Sorry, I tried my best but there are a few likes I couldn't cover.
Mostly the edge scenarios like stream not ending correctly.
I can open a follow up PR to improve the coverage by adding tests for existing uncovered lines. Hopefully that will be good enough to remove the exception.
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.
Sounds good. Thanks!
mathetake
left a comment
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.
Thank you so much for working on this. LGTM. can we add more tests to avoid having coverage exception?
Signed-off-by: Rohit Agrawal <[email protected]>
Thanks! I tried my best but there are a few likes I couldn't cover. I can open a follow up PR to improve the coverage by adding tests for existing uncovered lines. Hopefully that will be good enough to remove the exception. |
Signed-off-by: Rohit Agrawal <[email protected]>
Signed-off-by: Rohit Agrawal <[email protected]>
Description
The Async streamable HTTP client is supported by Envoy since long time. Using it, we can create multiple different HTTP streams to different backends at the same time and distribute requests between these backends.
This PR exposes this feature to dynamic modules. It could be used for developing MCP related dynamic modules.
Fix #42045
Commit Message: dynamic_module: add new streamable HTTP callout support
Additional Description: Add support for the Async streamable HTTP client in Dynamic Modules.
Risk Level: Low
Testing: Added Unit + Integration Tests
Docs Changes: Added
Release Notes: Added