spacewander commented on a change in pull request #3300:
URL: https://github.com/apache/apisix/pull/3300#discussion_r566531369
##########
File path: apisix/balancer/ewma.lua
##########
@@ -119,38 +137,75 @@ local function _trans_format(up_nodes)
return next(peers) and peers or nil
end
-
local function _ewma_find(ctx, up_nodes)
local peers
- local endpoint
- if not up_nodes
- or core.table.nkeys(up_nodes) == 0 then
+ if not up_nodes or nkeys(up_nodes) == 0 then
return nil, 'up_nodes empty'
end
- peers = lrucache_trans_format(ctx.upstream_key, ctx.upstream_version,
- _trans_format, up_nodes)
+ if ctx.balancer_tried_servers and ctx.balancer_tried_servers_count ==
nkeys(up_nodes) then
+ return nil, "all upstream servers tried"
+ end
+
+ peers = lrucache_trans_format(ctx.upstream_key, ctx.upstream_version,
_trans_format, up_nodes)
if not peers then
return nil, 'up_nodes trans error'
end
- if #peers > 1 then
- endpoint = pick_and_score(peers)
- else
- endpoint = peers[1]
+ local filtered_peers
+ for _, peer in ipairs(peers) do
+ if ctx.balancer_tried_servers then
+ if not ctx.balancer_tried_servers[get_upstream_name(peer)] then
+ if not filtered_peers then
+ filtered_peers = {}
+ end
+
+ table_insert(filtered_peers, peer)
+ end
+ end
end
- return endpoint.host .. ":" .. endpoint.port
-end
+ if not filtered_peers then
+ core.log.warn("all endpoints have been retried")
Review comment:
We don't need this branch as we already reject this case above:
```lua
if ctx.balancer_tried_servers and ctx.balancer_tried_servers_count ==
nkeys(up_nodes) then
return nil, "all upstream servers tried"
end
```
##########
File path: apisix/balancer/ewma.lua
##########
@@ -119,38 +137,75 @@ local function _trans_format(up_nodes)
return next(peers) and peers or nil
end
-
local function _ewma_find(ctx, up_nodes)
local peers
- local endpoint
- if not up_nodes
- or core.table.nkeys(up_nodes) == 0 then
+ if not up_nodes or nkeys(up_nodes) == 0 then
return nil, 'up_nodes empty'
end
- peers = lrucache_trans_format(ctx.upstream_key, ctx.upstream_version,
- _trans_format, up_nodes)
+ if ctx.balancer_tried_servers and ctx.balancer_tried_servers_count ==
nkeys(up_nodes) then
+ return nil, "all upstream servers tried"
+ end
+
+ peers = lrucache_trans_format(ctx.upstream_key, ctx.upstream_version,
_trans_format, up_nodes)
if not peers then
return nil, 'up_nodes trans error'
end
- if #peers > 1 then
- endpoint = pick_and_score(peers)
- else
- endpoint = peers[1]
+ local filtered_peers
+ for _, peer in ipairs(peers) do
+ if ctx.balancer_tried_servers then
Review comment:
This branch should move outside the for loop. And set the filtered_peers
to peers if not in retry.
##########
File path: apisix/balancer/ewma.lua
##########
@@ -35,73 +58,69 @@ local function decay_ewma(ewma, last_touched_at, rtt, now)
return ewma
end
-
local function store_stats(upstream, ewma, now)
local success, err, forcible = shm_last_touched_at:set(upstream, now)
if not success then
- core.log.error("balancer_ewma_last_touched_at:set failed ", err)
+ core.log.error("shm_last_touched_at:set failed: ", err)
end
if forcible then
- core.log.warn("balancer_ewma_last_touched_at:set valid items forcibly
overwritten")
+ core.log.warn("shm_last_touched_at:set valid items forcibly
overwritten")
end
success, err, forcible = shm_ewma:set(upstream, ewma)
if not success then
- core.log.error("balancer_ewma:set failed ", err)
+ core.log.error("shm_ewma:set failed: ", err)
end
if forcible then
- core.log.warn("balancer_ewma:set valid items forcibly overwritten")
+ core.log.warn("shm_ewma:set valid items forcibly overwritten")
end
end
-
local function get_or_update_ewma(upstream, rtt, update)
+ local lock_err = nil
+ if update then
+ lock_err = lock(upstream)
+ end
+
local ewma = shm_ewma:get(upstream) or 0
+ if lock_err ~= nil then
Review comment:
Why don't move this block inside the ` if update then`?
##########
File path: apisix/balancer/ewma.lua
##########
@@ -35,73 +57,69 @@ local function decay_ewma(ewma, last_touched_at, rtt, now)
return ewma
end
-
local function store_stats(upstream, ewma, now)
local success, err, forcible = shm_last_touched_at:set(upstream, now)
if not success then
- core.log.error("balancer_ewma_last_touched_at:set failed ", err)
+ core.log.error("shm_last_touched_at:set failed: ", err)
end
if forcible then
- core.log.warn("balancer_ewma_last_touched_at:set valid items forcibly
overwritten")
+ core.log.warn("shm_last_touched_at:set valid items forcibly
overwritten")
end
success, err, forcible = shm_ewma:set(upstream, ewma)
if not success then
- core.log.error("balancer_ewma:set failed ", err)
+ core.log.error("shm_ewma:set failed: ", err)
end
if forcible then
- core.log.warn("balancer_ewma:set valid items forcibly overwritten")
+ core.log.warn("shm_ewma:set valid items forcibly overwritten")
end
end
-
local function get_or_update_ewma(upstream, rtt, update)
+ local lock_err = nil
+ if update then
+ lock_err = lock(upstream)
+ end
+
local ewma = shm_ewma:get(upstream) or 0
+ if lock_err ~= nil then
+ return ewma, lock_err
+ end
+
local now = ngx_now()
local last_touched_at = shm_last_touched_at:get(upstream) or 0
ewma = decay_ewma(ewma, last_touched_at, rtt, now)
if not update then
- return ewma
+ return ewma, nil
end
store_stats(upstream, ewma, now)
- return ewma
+ unlock()
Review comment:
The `unlock()`'s err is not checked?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
