Skip to content

perf: simple setup upstream#8130

Merged
spacewander merged 18 commits into
apache:masterfrom
tzssangglass:miniup2
Oct 23, 2022
Merged

perf: simple setup upstream#8130
spacewander merged 18 commits into
apache:masterfrom
tzssangglass:miniup2

Conversation

@tzssangglass

@tzssangglass tzssangglass commented Oct 20, 2022

Copy link
Copy Markdown
Member

Description

Fixes # (issue)

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 first)

@tzssangglass tzssangglass changed the title adjust ai plugin priority perf: simple setup upstream Oct 20, 2022
membphis
membphis reviewed Oct 20, 2022
View reviewed changes
Comment thread t/debug/dynamic-hook.t Outdated
@tzssangglass
tzssangglass marked this pull request as ready for review October 21, 2022 01:20
membphis
membphis previously approved these changes Oct 21, 2022
View reviewed changes
Comment thread apisix/plugins/ai.lua Outdated
spacewander
spacewander reviewed Oct 21, 2022
View reviewed changes
Comment thread apisix/plugins/ai.lua Outdated
Comment thread apisix/plugins/ai.lua
balancer.enable_keepalive(60, 1000)
else
balancer.set_current_peer(server.host, server.port or 80)
end

@spacewander spacewander Oct 21, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can move:

if enable_keepalive then

to here?
Like:

if enable_keepalive then
    balancer.enable_keepalive(60, 1000)

@tzssangglass tzssangglass Oct 21, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe not, follow:

apisix/apisix/balancer.lua

Lines 291 to 338 in ecdc209

if enable_keepalive then
if not keepalive_pool then
if not default_keepalive_pool then
local local_conf = core.config.local_conf()
local up_keepalive_conf =
core.table.try_read_attr(local_conf, "nginx_config",
"http", "upstream")
default_keepalive_pool = {}
default_keepalive_pool.idle_timeout =
core.config_util.parse_time_unit(up_keepalive_conf.keepalive_timeout)
default_keepalive_pool.size = up_keepalive_conf.keepalive
default_keepalive_pool.requests = up_keepalive_conf.keepalive_requests
end
keepalive_pool = default_keepalive_pool
end
local idle_timeout = keepalive_pool.idle_timeout
local size = keepalive_pool.size
local requests = keepalive_pool.requests
core.table.clear(pool_opt)
pool_opt.pool_size = size
local scheme = up_conf.scheme
-- other TLS schemes don't use http balancer keepalive
if (scheme == "https" or scheme == "grpcs") then
local pool = server.host .. "#" .. server.port
local sni = ctx.var.upstream_host
pool = pool .. "#" .. sni
if up_conf.tls and up_conf.tls.client_cert then
pool = pool .. "#" .. up_conf.tls.client_cert
end
pool_opt.pool = pool
end
local ok, err = balancer.set_current_peer(server.host, server.port,
pool_opt)
if not ok then
return ok, err
end
return balancer.enable_keepalive(idle_timeout, requests)
end
return balancer.set_current_peer(server.host, server.port)

if enable_keepalive, would add pool_opt as the third argument to balancer.set_current_peer.

Do we fix the call to set_current_peer here and in balancer.lua, pass pool_opt even if there is disable enable_keepalive, otherwise it will all become non-persistent connection.

Comment thread apisix/plugins/ai.lua Outdated
Comment thread apisix/plugins/ai.lua Outdated
membphis
membphis reviewed Oct 21, 2022
View reviewed changes
Comment thread apisix/plugins/ai.lua
@spacewander
spacewander merged commit 3887162 into apache:master Oct 23, 2022
Liu-Junlin pushed a commit to Liu-Junlin/apisix that referenced this pull request Nov 4, 2022
@tzssangglass @Liu-Junlin
perf: simple setup upstream (apache#8130)
7d23cd9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@spacewander spacewander spacewander approved these changes
@membphis membphis membphis approved these changes
@soulbird soulbird Awaiting requested review from soulbird

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@tzssangglass @spacewander @membphis