-
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
Added utility methods to parse ORCA response headers from backends. #35422
Conversation
Signed-off-by: blake-snyder <blakesnyder@google.com>
Hi @blake-snyder, welcome and thank you for your contribution. We will try to review your Pull Request as quickly as possible. In the meantime, please take a look at the contribution guidelines if you have not done so already. |
/assign @adisuissa |
blake-snyder is not allowed to assign users. |
Switch string serialization util method to something compatible with all builds. Signed-off-by: blake-snyder <blakesnyder@google.com>
Failed checks appear to be related to cncf/xds#97 |
@blake-snyder some error handling code is not covered by tests and coverage requirement is not met. https://storage.googleapis.com/envoy-pr/4dacf1a/coverage/source/common/orca/orca_parser.cc.gcov.html /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.
Thanks!
Overall LGTM.
Can you please increase coverage (adding more tests)?
@wbpcode will you have the bandwidth to be the senior-maintainer reviewing the ORCA & new LB policy PRs? |
Signed-off-by: blake-snyder <blakesnyder@google.com>
Signed-off-by: blake-snyder <blakesnyder@google.com>
Signed-off-by: blake-snyder <blakesnyder@google.com>
…eaders. Change static constexpr variables to absl::string_view. Signed-off-by: blake-snyder <blakesnyder@google.com>
Signed-off-by: blake-snyder <blakesnyder@google.com>
Signed-off-by: blake-snyder <blakesnyder@google.com>
/retest |
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!
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 overall with only two nit comments. Thanks so much for your patience, work, and update.
Signed-off-by: blake-snyder <blakesnyder@google.com>
Signed-off-by: blake-snyder <blakesnyder@google.com>
/retest |
…nds. (envoyproxy#35422) Signed-off-by: blake-snyder <blakesnyder@google.com> Signed-off-by: asingh-g <abhisinghx@google.com>
Commit Message: Add support for multiple formats of ORCA headers. Additional Description: Add support for multiple formats of ORCA headers. ORCA parsing introduced in #35422 [Original Design Proposal](#6614) [Using ORCA load reports in Envoy](https://docs.google.com/document/d/1gb_2pcNnEzTgo1EJ6w1Ol7O-EH-O_Ysu5o215N9MTAg/edit#heading=h.bi4e79pb39fe) Risk Level: Low Testing: See included unit tests. Docs Changes: N/A Release Notes: N/A Platform Specific Features: JSON format unsupported on Mobile. CC @efimki @adisuissa @wbpcode --------- Signed-off-by: blake-snyder <blakesnyder@google.com>
Commit Message: Add support for multiple formats of ORCA headers. Additional Description: Add support for multiple formats of ORCA headers. ORCA parsing introduced in envoyproxy#35422 [Original Design Proposal](envoyproxy#6614) [Using ORCA load reports in Envoy](https://docs.google.com/document/d/1gb_2pcNnEzTgo1EJ6w1Ol7O-EH-O_Ysu5o215N9MTAg/edit#heading=h.bi4e79pb39fe) Risk Level: Low Testing: See included unit tests. Docs Changes: N/A Release Notes: N/A Platform Specific Features: JSON format unsupported on Mobile. CC @efimki @adisuissa @wbpcode --------- Signed-off-by: blake-snyder <blakesnyder@google.com> Signed-off-by: Steven Jin Xuan <sjinxuan@microsoft.com>
Commit Message: Add utility methods to parse ORCA response headers from backends.
Additional Description: Add utility methods to parse ORCA response headers from backends.
parseOrcaLoadReportHeaders
will be called from theRouterFilter
.Open Request Cost Aggregation (ORCA) Design Proposal
Using ORCA load reports in Envoy
Risk Level: Low
Testing: See included unit tests.
Docs Changes: Included in PR
Release Notes: N/A
Platform Specific Features: JSON formatted header not supported on Envoy Mobile
CC @efimki @AndresGuedez