Skip to content

perf: allow skip body filter#8149

Merged
spacewander merged 5 commits into
apache:masterfrom
tzssangglass:skip
Oct 24, 2022
Merged

perf: allow skip body filter#8149
spacewander merged 5 commits into
apache:masterfrom
tzssangglass:skip

Conversation

@tzssangglass

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)

Comment thread apisix/plugins/ai.lua
local ngx = ngx
local is_http = ngx.config.subsystem == "http"
local enable_keepalive = balancer.enable_keepalive and is_http
local is_apisix_or, response = pcall(require, "resty.apisix.response")

@membphis membphis Oct 23, 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.

this should be a core API to get the version of APISIX runtime

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.

what do you thing? @spacewander

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.

You mean move pcall(require, "resty.apisix.response") on apisix core level?

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.

It is possible but not valuable to do this, as we don't want to support multiple versions of APISIX-Base, only support the latest APISIX-Base. Maybe a simple API to detect if apisix-base / openresty is used is enough.

Comment thread apisix/plugins/ai.lua Outdated
end

if is_apisix_or then
local ok, err = response.skip_header_filter_by_lua()

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.

Only the current request skips the ***_filter, all right?

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.

no, it effects global

Comment thread apisix/plugins/ai.lua
end

if is_apisix_or then
local ok, err = response.skip_body_filter_by_lua()

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.

No skip_header_filter_by_lua?

@tzssangglass tzssangglass Oct 23, 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.

cancel skip_header_filter_by_lua, because we add APISIX/2.99in the header_filter phase, and enable_server_tokens = false cannot be used as a flag to skip header_filter_by_lua because it only removes the version in server token, not delete the server token unless we add another switch for removing the server token completely.

@tzssangglass
tzssangglass marked this pull request as ready for review October 24, 2022 01:50
@tzssangglass tzssangglass changed the title perf: allow skip header filter and body filter perf: allow skip body filter Oct 24, 2022
membphis
membphis previously approved these changes Oct 24, 2022
Comment thread t/debug/dynamic-hook.t Outdated
Comment thread t/debug/dynamic-hook.t Outdated
spacewander
spacewander previously approved these changes Oct 24, 2022
@spacewander
spacewander merged commit fa654ed into apache:master Oct 24, 2022
Liu-Junlin pushed a commit to Liu-Junlin/apisix that referenced this pull request Nov 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants