[GitHub] [apisix] membphis commented on a diff in pull request #8130: perf: simple setup upstream

2022-10-21 Thread GitBox 


membphis commented on code in PR #8130:
URL: https://github.com/apache/apisix/pull/8130#discussion_r1001516411


##
apisix/plugins/ai.lua:
##
@@ -100,33 +108,120 @@ local function gen_get_cache_key_func(route_flags)
 end
 
 
+local function ai_upstream()
+core.log.info("enable sample upstream")
+end
+
+
+local pool_opt = { pool_size = 320 }
+local function ai_balancer_run(route)
+local server = route.value.upstream.nodes[1]
+if enable_keepalive then
+local ok, err = balancer.set_current_peer(server.host, server.port or 
80, pool_opt)
+if not ok then
+core.log.error("failed to set server peer [", server.host, ":",
+   server.port, "] err: ", err)
+return ok, err
+end
+balancer.enable_keepalive(60, 1000)

Review Comment:
   We should enable `keepalive` by default



-- 
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-unsubscr...@apisix.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [apisix] membphis commented on a diff in pull request #8130: perf: simple setup upstream

2022-10-20 Thread GitBox 


membphis commented on code in PR #8130:
URL: https://github.com/apache/apisix/pull/8130#discussion_r1001431875


##
apisix/plugins/ai.lua:
##
@@ -100,33 +108,120 @@ local function gen_get_cache_key_func(route_flags)
 end
 
 
+local function ai_upstream()
+core.log.info("enable sample upstream")
+end
+
+
+local pool_opt = { pool_size = 320 }
+local function ai_balancer_run(route)
+local server = route.value.upstream.nodes[1]
+if enable_keepalive then
+local ok, err = balancer.set_current_peer(server.host, server.port or 
80, pool_opt)
+if not ok then
+core.log.error("failed to set server peer [", server.host, ":",
+   server.port, "] err: ", err)
+return ok, err
+end
+balancer.enable_keepalive(60, 1000)
+else
+balancer.set_current_peer(server.host, server.port or 80)
+end
+end
+
 local function routes_analyze(routes)
 -- TODO: need to add a option in config.yaml to enable this 
feature(default is true)
-local route_flags = core.table.new(0, 2)
+local route_flags = core.table.new(0, 5)
+local route_up_flags = core.table.new(0, 8)
 for _, route in ipairs(routes) do
-if route.methods then
-route_flags["methods"] = true
-end
+if type(route) == "table" then

Review Comment:
   we can change it to a better style, which is simpler:
   
   ```lua
   local route_flags = {}
   
   for _, route in ipairs(routes) do
   for key, value in pairs(route.value) do
   -- collect route flags
   if key == "host" or key = "hosts" then
   route_flags["host"] = true
   elseif key == "remote_addr" or key = "remote_addrs" then
   route_flags["remote_addr"] = true
   else
   route_flags[key] = true
   end
   
   -- ... ...
   end
   
   end
   ```



-- 
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-unsubscr...@apisix.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [apisix] membphis commented on a diff in pull request #8130: perf: simple setup upstream

2022-10-20 Thread GitBox 


membphis commented on code in PR #8130:
URL: https://github.com/apache/apisix/pull/8130#discussion_r1001391139


##
apisix/plugins/ai.lua:
##
@@ -100,33 +108,120 @@ local function gen_get_cache_key_func(route_flags)
 end
 
 
+local function ai_upstream()
+core.log.info("enable sample upstream")
+end
+
+
+local pool_opt = { pool_size = 320 }
+local function ai_balancer_run(route)
+local server = route.value.upstream.nodes[1]
+if enable_keepalive then
+local ok, err = balancer.set_current_peer(server.host, server.port or 
80, pool_opt)
+if not ok then
+core.log.error("failed to set server peer [", server.host, ":",
+   server.port, "] err: ", err)
+return ok, err
+end
+balancer.enable_keepalive(60, 1000)
+else
+balancer.set_current_peer(server.host, server.port or 80)
+end
+end
+
 local function routes_analyze(routes)
 -- TODO: need to add a option in config.yaml to enable this 
feature(default is true)

Review Comment:
   I think we can remove this line now



-- 
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-unsubscr...@apisix.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [apisix] membphis commented on a diff in pull request #8130: perf: simple setup upstream

2022-10-20 Thread GitBox 


membphis commented on code in PR #8130:
URL: https://github.com/apache/apisix/pull/8130#discussion_r1000589930


##
t/debug/dynamic-hook.t:
##
@@ -405,7 +405,8 @@ hook_test:  # module and function list, 
name: hook_test
 "uri": "/hello",
 "upstream": {
 "nodes": {
-"127.0.0.1:1980": 1
+"127.0.0.1:1980": 1,

Review Comment:
   why do we need to change this?



-- 
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-unsubscr...@apisix.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org