Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
perf(pdk): refine with optional cache to improvement performance #13981
base: master
Are you sure you want to change the base?
perf(pdk): refine with optional cache to improvement performance #13981
Changes from all commits
c4aebf1
e5626cd
7c9ea30
ca17150
e0dc602
78cb953
e46dffb
ec8abf2
9c7520a
f4ce86a
bcdc538
40f7218
71e2a62
d5c781b
a64667b
f140e01
16b159c
b40a178
d21ef10
edd040a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we keep the original code here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ngx.header
is the better way to get a single response header, and it returns the same type of values asget_headers()[name]
: ifget_headers()[name]
get a table,ngx.header
will get a table similarly(unlikengx.var.sent_http_
which returns comma separated string). So we make this change for better performance.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you confirm it is ok?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a tricky point, here we want to use
headers
result as param to reduce duplicatedget_header
call. Althoughorigin
header should not be a multiple value logically, but users may still sendhttp http://xxx/yyy Origin:o1 Origin:o2
from downstream which doesn't quite make sense as I see(also I feel like twoContent-Type
header in one http request is also not quite logically correct...).But anyway in this case
kong.request.get_header("origin")
will get only first valueo1
andget_headers()["origin"]
will get{o1, o2}
.So the
get_header
API guarantees we only get a single value no matter it's multiple value or not. But in the other side, its pointless to runget_header
multiple times since the header has not been changed. Maybe we could just change this to:type(headers["origin"]) == "table" and headers["origin"][1] or headers["origin"]
which is the implementation to make header value always single(see https://github.com/Kong/kong/blob/master/kong/tools/http.lua#L564).
@bungle @chronolaw
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you confirm it is ok?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you confirm it is ok?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a new feature?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not a new feature, just a refine, since we use
ngx.header[name]
to replace_RESPONSE.get_headers()[name]
in theget_header
implementation, the max headers limitation brought byget_headers()
will not exist anymore.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this extends the capability of this PDK function. Shall we consider this a new feature?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thinks it's not needed to consider this a new feature, because
that means we have no interface changes and no documentation changes from user's perspective.