-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
router: only cache single route for the active request #26045
Conversation
Signed-off-by: wbpcode <wangbaiping@corp.netease.com>
Signed-off-by: wbpcode <wangbaiping@corp.netease.com>
Signed-off-by: wbpcode <wangbaiping@corp.netease.com>
Signed-off-by: wbpcode <wangbaiping@corp.netease.com>
Signed-off-by: wbpcode <wangbaiping@corp.netease.com>
Signed-off-by: wbpcode <wangbaiping@corp.netease.com>
/wait on CI - I think the router drop is real |
Will to write some tests to improve the coverage of the It would be great if more experts could help to review this PR. This lifetime change always be dangerous patch. |
cc @JakeCooper Thanks for your feedback. Note this patch is no completed and it couldn't be used in the production. Especially if scoped rds & ondemand rds are used by you. |
Signed-off-by: wbpcode <wangbaiping@corp.netease.com>
I rechecked the code of ondemand rds. Seems the ondemand callback is managed by the route config provider. So this patch will not effect it. |
Signed-off-by: wbpcode <wangbaiping@corp.netease.com>
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, the code changes LGTM, but would add a few tests that validate that the new behavior is maintained (if the config is changed during execution time, the new one is latched onto).
IMO there is a fundamental question here: when should the filter-manager latch to the route table (see Matt's comment?
Prior to this PR the routing table is latched during the decodeHeaders()
call.
This PR changes it to latch later when route()
is called.
Changing the routing table view in the midst of routing is possible, but it should be reviewed by someone that understands the details of the router.
Also cc @envoyproxy/senior-maintainers to chime in on this.
Another alternative to consider is to explicitly release the routing table, once a request "realizes" that the route is fixed (i.e., no need to reroute). It may be a bit easier to understand the code, and clearly find the cases where there could be an old-new view conflict of the routing table. WDYT?
In fact, we still will get the route table in the
Sounds a good idea. Then we can keep current behaviors completely. Typically, we can do this in the |
…oute-config-opt
Signed-off-by: wbpcode <wangbaiping@corp.netease.com>
Signed-off-by: wbpcode <wangbaiping@corp.netease.com>
Signed-off-by: wbpcode <wangbaiping@corp.netease.com>
…oute-config-opt
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 with a few comments, thank you for tackling this tough problem! Awesome.
/wait
Co-authored-by: Matt Klein <mattklein123@gmail.com> Signed-off-by: code <wangbaiping@corp.netease.com>
Co-authored-by: Matt Klein <mattklein123@gmail.com> Signed-off-by: code <wangbaiping@corp.netease.com>
Co-authored-by: Matt Klein <mattklein123@gmail.com> Signed-off-by: code <wangbaiping@corp.netease.com>
Signed-off-by: wbpcode <wangbaiping@corp.netease.com>
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.
Awesome work. LGTM!
…oute-config-opt
Signed-off-by: wbpcode <wangbaiping@corp.netease.com>
Signed-off-by: wbpcode <wangbaiping@corp.netease.com>
Hi, @mattklein123 could you give a stamp again? 😸 @ Conflict resolved and minor update to the changelog format. |
) * router: only cache single route for the active request Signed-off-by: wbpcode <wangbaiping@corp.netease.com> In the previous impl of the route table, every active request will keep a reference of the whole route table. If there route table is updated frequently and there are some long-live http request, the old versions route tables cannot be released timely and increase the overhead of memory. This PR try to fix this problem by cache single route for active request. See envoyproxy#20800 for more detailed background. Risk Level: High. Core code patch. Change the lifetime of route table. Testing: Added/fixed. Docs Changes: n/a. Release Notes: n/a. Platform Specific Features: n/a. Optional Runtime guard: envoy.reloadable_features.prohibit_route_refresh_after_response_headers_sent --------- Signed-off-by: wbpcode <wangbaiping@corp.netease.com> Signed-off-by: code <wangbaiping@corp.netease.com> Co-authored-by: Matt Klein <mattklein123@gmail.com> Signed-off-by: River Phillips <riverphillips1@gmail.com>
Commit Message: router: only cache single route for the active request
Additional Description:
In the previous impl of the route table, every active request will keep a reference of the whole route table. If there route table is updated frequently and there are some long-live http request, the old versions route tables cannot be released timely and increase the overhead of memory.
This PR try to fix this problem by cache single route for active request.
See #20800 for more detailed background.
Risk Level: High. Core code patch. Change the lifetime of route table.
Testing: Added/fixed.
Docs Changes: n/a.
Release Notes: n/a.
Platform Specific Features: n/a.