You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by "ranxuxin (via GitHub)" <gi...@apache.org> on 2023/04/07 06:37:56 UTC
[GitHub] [apisix] ranxuxin opened a new pull request, #9259: avoid creating the whole radixtree while changing one route
ranxuxin opened a new pull request, #9259:
URL: https://github.com/apache/apisix/pull/9259
### Description
It improves the performance of apisix when there're more than 10 thousand routes and modifying one route. The new codes just modify the route data in routes array or the relative node of radixtree instead of creating the whole radixtree while one route was changed.
After tesing as users at 1000 concurrency using ab tool while changing a route every one second, the overhead of cpu time is only 7%-8%. On the other hand, the apisix 2.9 costs near 100% cpu time.
This patch is based the commit c924eb1cded079ec0d6279b211466b7d84a91ec2 in improveCreateRadixtree branch of apisix 2.9 .
Fixes #9140
### Checklist
- [ ] I have explained the need for this PR and the problem it solves
- [ ] I have explained the changes or the new features added to this PR
- [ ] I have added tests corresponding to this change
- [ ] I have updated the documentation to reflect this change
- [ ] I have verified that this change is backward compatible (If not, please discuss on the [APISIX mailing list](https://github.com/apache/apisix/tree/master#community) first)
<!--
Note
1. Mark the PR as draft until it's ready to be reviewed.
2. Always add/update tests for any changes unless you have a good reason.
3. Always update the documentation to reflect the changes made in the PR.
4. Make a new commit to resolve conversations instead of `push -f`.
5. To resolve merge conflicts, merge master instead of rebasing.
6. Use "request review" to notify the reviewer after making changes.
7. Only a reviewer can mark a conversation as resolved.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] kingluo commented on a diff in pull request #9259: avoid creating the whole radixtree while changing one route
Posted by "kingluo (via GitHub)" <gi...@apache.org>.
kingluo commented on code in PR #9259:
URL: https://github.com/apache/apisix/pull/9259#discussion_r1160504439
##########
apisix/core/config_etcd.lua:
##########
@@ -463,6 +480,76 @@ local function sync_data(self)
end
self.conf_version = self.conf_version + 1
+
+ if self.key ~= "/apisix/routes" then
+ goto CONTINUE
+ end
+
+ local route
+ if res.value then
+ local status = table.try_read_attr(res, "value", "status")
+ if status and status == 0 then
+ goto CONTINUE
+ end
+
+ local filter_fun, err
+ if res.value.filter_func then
+ filter_fun, err = loadstring(
+ "return " .. res.value.filter_func,
+ "router#" .. res.value.id
+ )
+ if not filter_fun then
+ log.error("failed to load filter function: ", err, " route id", res.value.id)
+ goto CONTINUE
+ end
+
+ filter_fun = filter_fun()
+ end
+
+ route = {
+ id = res.value.id,
+ paths = res.value.uris or res.value.uri,
+ methods = res.value.methods,
+ priority = res.value.priority,
+ hosts = res.value.hosts or res.value.host,
+ remote_addrs = res.value.remote_addrs or res.value.remote_addr,
+ vars = res.value.vars,
+ filter_fun = filter_fun,
+ handler = function(api_ctx, match_opts)
+ api_ctx.matched_params = nil
+ api_ctx.matched_route = res
+ api_ctx.curr_req_matched = match_opts.matched
+ end
+ }
+ end
+
+ local router_opts = {
+ no_param_match = true
+ }
+ local router_mod = require("apisix.router")
+ if op == 2 then
+ log.notice("create routes watched from etcd into radixtree.", json.encode(res))
+ err = router_mod.uri_router:add_route(route, router_opts)
Review Comment:
Where's the source code of the new methods of the router, e.g. `add_route`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] kingluo commented on a diff in pull request #9259: avoid creating the whole radixtree while changing one route
Posted by "kingluo (via GitHub)" <gi...@apache.org>.
kingluo commented on code in PR #9259:
URL: https://github.com/apache/apisix/pull/9259#discussion_r1160514763
##########
apisix/core/config_etcd.lua:
##########
@@ -463,6 +480,76 @@ local function sync_data(self)
end
self.conf_version = self.conf_version + 1
+
+ if self.key ~= "/apisix/routes" then
+ goto CONTINUE
+ end
+
+ local route
+ if res.value then
+ local status = table.try_read_attr(res, "value", "status")
+ if status and status == 0 then
+ goto CONTINUE
+ end
+
+ local filter_fun, err
+ if res.value.filter_func then
+ filter_fun, err = loadstring(
+ "return " .. res.value.filter_func,
+ "router#" .. res.value.id
+ )
+ if not filter_fun then
+ log.error("failed to load filter function: ", err, " route id", res.value.id)
+ goto CONTINUE
+ end
+
+ filter_fun = filter_fun()
+ end
+
+ route = {
+ id = res.value.id,
+ paths = res.value.uris or res.value.uri,
+ methods = res.value.methods,
+ priority = res.value.priority,
+ hosts = res.value.hosts or res.value.host,
+ remote_addrs = res.value.remote_addrs or res.value.remote_addr,
+ vars = res.value.vars,
+ filter_fun = filter_fun,
+ handler = function(api_ctx, match_opts)
+ api_ctx.matched_params = nil
+ api_ctx.matched_route = res
+ api_ctx.curr_req_matched = match_opts.matched
+ end
+ }
+ end
+
+ local router_opts = {
+ no_param_match = true
+ }
+ local router_mod = require("apisix.router")
+ if op == 2 then
+ log.notice("create routes watched from etcd into radixtree.", json.encode(res))
+ err = router_mod.uri_router:add_route(route, router_opts)
Review Comment:
Please refer to my suggestion below about code refactoring, thanks.
We need full code for contribution, as well as corresponding tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] ranxuxin commented on pull request #9259: avoid creating the whole radixtree while changing one route
Posted by "ranxuxin (via GitHub)" <gi...@apache.org>.
ranxuxin commented on PR #9259:
URL: https://github.com/apache/apisix/pull/9259#issuecomment-1500052473
@kingluo I've implemented the lua API to export raxRemove. And compile the codes into dynamic shared library.
To filter non route keys in sync_data() method, I've already filtered them such as "/apisix/upstreams". the codes is in config_etcd.lua line 484
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] kingluo commented on pull request #9259: avoid creating the whole radixtree while changing one route
Posted by "kingluo (via GitHub)" <gi...@apache.org>.
kingluo commented on PR #9259:
URL: https://github.com/apache/apisix/pull/9259#issuecomment-1500029858
In fact, we should do below steps to implement incremental route synchronization:
* export `raxRemove` as lua API
https://github.com/api7/lua-resty-radixtree/blob/5364cbfe742a5a11f45c16d66b38d7e7b8bd969c/src/rax.c#L1020
```c
/* Remove the specified item. Returns 1 if the item was found and
* deleted, 0 otherwise. */
int raxRemove(rax *rax, unsigned char *s, size_t len, void **old)
```
Then:
Delete a route in etcd equals removing the old route item from the radix tree.
Update a route in etcd equals removing the old route item and inserting a new one into the radix tree.
* update the route in filter, not in config_etcd, because that handles watching for all resources, not only routes
https://github.com/apache/apisix/blob/8805fc543f03bf04eec656d69eb46ed2377db1ce/apisix/router.lua#L30-L45
* disable full update of radix tree
https://github.com/apache/apisix/blob/8805fc543f03bf04eec656d69eb46ed2377db1ce/apisix/http/router/radixtree_uri.lua#L31-L41
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] kingluo commented on pull request #9259: avoid creating the whole radixtree while changing one route
Posted by "kingluo (via GitHub)" <gi...@apache.org>.
kingluo commented on PR #9259:
URL: https://github.com/apache/apisix/pull/9259#issuecomment-1500059762
> @kingluo I've implemented the lua API to export raxRemove. And compile the codes into dynamic shared library. To filter non route keys in sync_data() method, I've already filtered them such as "/apisix/upstreams". the codes is in config_etcd.lua line 484
>
> To update one route, if the path of new and old route is the same, find the route in routes array by route id, and assign the new route data to the correct element of the routes array. Then delete routes with old path, insert new routes with new path.
To make a good code practice, please apply the refactoring in the suitable scope, i.e. filter.
Anyways, please submit a full but minimum code change upon the master branch instead, remove irrelevant changes, and provide test cases.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] ranxuxin commented on a diff in pull request #9259: avoid creating the whole radixtree while changing one route
Posted by "ranxuxin (via GitHub)" <gi...@apache.org>.
ranxuxin commented on code in PR #9259:
URL: https://github.com/apache/apisix/pull/9259#discussion_r1160517192
##########
apisix/core/config_etcd.lua:
##########
@@ -463,6 +480,76 @@ local function sync_data(self)
end
self.conf_version = self.conf_version + 1
+
+ if self.key ~= "/apisix/routes" then
+ goto CONTINUE
+ end
+
+ local route
+ if res.value then
+ local status = table.try_read_attr(res, "value", "status")
+ if status and status == 0 then
+ goto CONTINUE
+ end
+
+ local filter_fun, err
+ if res.value.filter_func then
+ filter_fun, err = loadstring(
+ "return " .. res.value.filter_func,
+ "router#" .. res.value.id
+ )
+ if not filter_fun then
+ log.error("failed to load filter function: ", err, " route id", res.value.id)
+ goto CONTINUE
+ end
+
+ filter_fun = filter_fun()
+ end
+
+ route = {
+ id = res.value.id,
+ paths = res.value.uris or res.value.uri,
+ methods = res.value.methods,
+ priority = res.value.priority,
+ hosts = res.value.hosts or res.value.host,
+ remote_addrs = res.value.remote_addrs or res.value.remote_addr,
+ vars = res.value.vars,
+ filter_fun = filter_fun,
+ handler = function(api_ctx, match_opts)
+ api_ctx.matched_params = nil
+ api_ctx.matched_route = res
+ api_ctx.curr_req_matched = match_opts.matched
+ end
+ }
+ end
+
+ local router_opts = {
+ no_param_match = true
+ }
+ local router_mod = require("apisix.router")
+ if op == 2 then
+ log.notice("create routes watched from etcd into radixtree.", json.encode(res))
+ err = router_mod.uri_router:add_route(route, router_opts)
Review Comment:
I've implemented the method to delete the node in radixtree. The share library is in apisix/deps/lib/lua/5.1/librestyradixtree.so. It can be download in https://pan.baidu.com/s/1_xb5MrHsZx4v519wSCFNkg enter code: 5566
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] ranxuxin001 closed pull request #9259: avoid creating the whole radixtree while changing one route
Posted by "ranxuxin001 (via GitHub)" <gi...@apache.org>.
ranxuxin001 closed pull request #9259: avoid creating the whole radixtree while changing one route
URL: https://github.com/apache/apisix/pull/9259
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] ranxuxin commented on a diff in pull request #9259: avoid creating the whole radixtree while changing one route
Posted by "ranxuxin (via GitHub)" <gi...@apache.org>.
ranxuxin commented on code in PR #9259:
URL: https://github.com/apache/apisix/pull/9259#discussion_r1160512693
##########
apisix/core/config_etcd.lua:
##########
@@ -463,6 +480,76 @@ local function sync_data(self)
end
self.conf_version = self.conf_version + 1
+
+ if self.key ~= "/apisix/routes" then
+ goto CONTINUE
+ end
+
+ local route
+ if res.value then
+ local status = table.try_read_attr(res, "value", "status")
+ if status and status == 0 then
+ goto CONTINUE
+ end
+
+ local filter_fun, err
+ if res.value.filter_func then
+ filter_fun, err = loadstring(
+ "return " .. res.value.filter_func,
+ "router#" .. res.value.id
+ )
+ if not filter_fun then
+ log.error("failed to load filter function: ", err, " route id", res.value.id)
+ goto CONTINUE
+ end
+
+ filter_fun = filter_fun()
+ end
+
+ route = {
+ id = res.value.id,
+ paths = res.value.uris or res.value.uri,
+ methods = res.value.methods,
+ priority = res.value.priority,
+ hosts = res.value.hosts or res.value.host,
+ remote_addrs = res.value.remote_addrs or res.value.remote_addr,
+ vars = res.value.vars,
+ filter_fun = filter_fun,
+ handler = function(api_ctx, match_opts)
+ api_ctx.matched_params = nil
+ api_ctx.matched_route = res
+ api_ctx.curr_req_matched = match_opts.matched
+ end
+ }
+ end
+
+ local router_opts = {
+ no_param_match = true
+ }
+ local router_mod = require("apisix.router")
+ if op == 2 then
+ log.notice("create routes watched from etcd into radixtree.", json.encode(res))
+ err = router_mod.uri_router:add_route(route, router_opts)
Review Comment:
add_route and other methods to operate radixtree is in this path apisix/deps/share/lua/5.1/resty/radixtree.lua .
the source code is here. https://pan.baidu.com/s/1L2dxC67PoaJ3brsF-sDDXQ enter code: 6688
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] ranxuxin commented on a diff in pull request #9259: avoid creating the whole radixtree while changing one route
Posted by "ranxuxin (via GitHub)" <gi...@apache.org>.
ranxuxin commented on code in PR #9259:
URL: https://github.com/apache/apisix/pull/9259#discussion_r1160517192
##########
apisix/core/config_etcd.lua:
##########
@@ -463,6 +480,76 @@ local function sync_data(self)
end
self.conf_version = self.conf_version + 1
+
+ if self.key ~= "/apisix/routes" then
+ goto CONTINUE
+ end
+
+ local route
+ if res.value then
+ local status = table.try_read_attr(res, "value", "status")
+ if status and status == 0 then
+ goto CONTINUE
+ end
+
+ local filter_fun, err
+ if res.value.filter_func then
+ filter_fun, err = loadstring(
+ "return " .. res.value.filter_func,
+ "router#" .. res.value.id
+ )
+ if not filter_fun then
+ log.error("failed to load filter function: ", err, " route id", res.value.id)
+ goto CONTINUE
+ end
+
+ filter_fun = filter_fun()
+ end
+
+ route = {
+ id = res.value.id,
+ paths = res.value.uris or res.value.uri,
+ methods = res.value.methods,
+ priority = res.value.priority,
+ hosts = res.value.hosts or res.value.host,
+ remote_addrs = res.value.remote_addrs or res.value.remote_addr,
+ vars = res.value.vars,
+ filter_fun = filter_fun,
+ handler = function(api_ctx, match_opts)
+ api_ctx.matched_params = nil
+ api_ctx.matched_route = res
+ api_ctx.curr_req_matched = match_opts.matched
+ end
+ }
+ end
+
+ local router_opts = {
+ no_param_match = true
+ }
+ local router_mod = require("apisix.router")
+ if op == 2 then
+ log.notice("create routes watched from etcd into radixtree.", json.encode(res))
+ err = router_mod.uri_router:add_route(route, router_opts)
Review Comment:
I've implemented the method to delete the node in radixtree. The share library is in apisix/deps/lib/lua/5.1/librestyradixtree.so. It can be download in https://pan.baidu.com/s/1Ci73Da_NSoC2bTietEfQEw enter code: 5566
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] kingluo commented on a diff in pull request #9259: avoid creating the whole radixtree while changing one route
Posted by "kingluo (via GitHub)" <gi...@apache.org>.
kingluo commented on code in PR #9259:
URL: https://github.com/apache/apisix/pull/9259#discussion_r1160514763
##########
apisix/core/config_etcd.lua:
##########
@@ -463,6 +480,76 @@ local function sync_data(self)
end
self.conf_version = self.conf_version + 1
+
+ if self.key ~= "/apisix/routes" then
+ goto CONTINUE
+ end
+
+ local route
+ if res.value then
+ local status = table.try_read_attr(res, "value", "status")
+ if status and status == 0 then
+ goto CONTINUE
+ end
+
+ local filter_fun, err
+ if res.value.filter_func then
+ filter_fun, err = loadstring(
+ "return " .. res.value.filter_func,
+ "router#" .. res.value.id
+ )
+ if not filter_fun then
+ log.error("failed to load filter function: ", err, " route id", res.value.id)
+ goto CONTINUE
+ end
+
+ filter_fun = filter_fun()
+ end
+
+ route = {
+ id = res.value.id,
+ paths = res.value.uris or res.value.uri,
+ methods = res.value.methods,
+ priority = res.value.priority,
+ hosts = res.value.hosts or res.value.host,
+ remote_addrs = res.value.remote_addrs or res.value.remote_addr,
+ vars = res.value.vars,
+ filter_fun = filter_fun,
+ handler = function(api_ctx, match_opts)
+ api_ctx.matched_params = nil
+ api_ctx.matched_route = res
+ api_ctx.curr_req_matched = match_opts.matched
+ end
+ }
+ end
+
+ local router_opts = {
+ no_param_match = true
+ }
+ local router_mod = require("apisix.router")
+ if op == 2 then
+ log.notice("create routes watched from etcd into radixtree.", json.encode(res))
+ err = router_mod.uri_router:add_route(route, router_opts)
Review Comment:
Please refer to my suggestion below about code refacotring, thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] ranxuxin commented on pull request #9259: avoid creating the whole radixtree while changing one route
Posted by "ranxuxin (via GitHub)" <gi...@apache.org>.
ranxuxin commented on PR #9259:
URL: https://github.com/apache/apisix/pull/9259#issuecomment-1500078906
> @ranxuxin May I have your contact? We wish to understand more detailed business requirements. cc @membphis
please add qq 1507204032
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] kingluo commented on pull request #9259: avoid creating the whole radixtree while changing one route
Posted by "kingluo (via GitHub)" <gi...@apache.org>.
kingluo commented on PR #9259:
URL: https://github.com/apache/apisix/pull/9259#issuecomment-1500074498
@ranxuxin May I have your contact? We wish to understand more detailed business requirements. cc @membphis
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] ranxuxin commented on a diff in pull request #9259: avoid creating the whole radixtree while changing one route
Posted by "ranxuxin (via GitHub)" <gi...@apache.org>.
ranxuxin commented on code in PR #9259:
URL: https://github.com/apache/apisix/pull/9259#discussion_r1160517192
##########
apisix/core/config_etcd.lua:
##########
@@ -463,6 +480,76 @@ local function sync_data(self)
end
self.conf_version = self.conf_version + 1
+
+ if self.key ~= "/apisix/routes" then
+ goto CONTINUE
+ end
+
+ local route
+ if res.value then
+ local status = table.try_read_attr(res, "value", "status")
+ if status and status == 0 then
+ goto CONTINUE
+ end
+
+ local filter_fun, err
+ if res.value.filter_func then
+ filter_fun, err = loadstring(
+ "return " .. res.value.filter_func,
+ "router#" .. res.value.id
+ )
+ if not filter_fun then
+ log.error("failed to load filter function: ", err, " route id", res.value.id)
+ goto CONTINUE
+ end
+
+ filter_fun = filter_fun()
+ end
+
+ route = {
+ id = res.value.id,
+ paths = res.value.uris or res.value.uri,
+ methods = res.value.methods,
+ priority = res.value.priority,
+ hosts = res.value.hosts or res.value.host,
+ remote_addrs = res.value.remote_addrs or res.value.remote_addr,
+ vars = res.value.vars,
+ filter_fun = filter_fun,
+ handler = function(api_ctx, match_opts)
+ api_ctx.matched_params = nil
+ api_ctx.matched_route = res
+ api_ctx.curr_req_matched = match_opts.matched
+ end
+ }
+ end
+
+ local router_opts = {
+ no_param_match = true
+ }
+ local router_mod = require("apisix.router")
+ if op == 2 then
+ log.notice("create routes watched from etcd into radixtree.", json.encode(res))
+ err = router_mod.uri_router:add_route(route, router_opts)
Review Comment:
I've implemented the method to delete the node in radixtree. The share library is in apisix/deps/lib/lua/5.1/librestyradixtree.so. It can be download in https://pan.baidu.com/s/1F0344pwPFx3pzfbNpv6Nzw enter code: 5566
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org