-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
fix(request-id): file descriptor overflow when use request-id plugin nanoid algorithm #11060
base: master
Are you sure you want to change the base?
Conversation
76abeb0
to
80a9d1c
Compare
@@ -51,6 +51,22 @@ local schema = { | |||
} | |||
}, | |||
default = {} | |||
}, | |||
nanoid = { |
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.
why is this block needed when just replacing the dependency would fix the issue?
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.
The new dependency provides more controllable parameters and more customized functions, so parameter options are added,just like range_id
apisix/plugins/request-id.lua
Outdated
if conf.algorithm == "nanoid" then | ||
return nanoid.safe_simple() | ||
return nanoid.generate(conf.nanoid.length,conf.nanoid.char_set) |
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.
space needed after comma
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.
space needed after comma
OK
[[{ | ||
"plugins": { | ||
"request-id": { | ||
"algorithm": "nanoid" |
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.
your PR suggests that usage method/configuration for nanoid has changed. Which means existing users will face incompatibility issues when upgrading to the newer version. We should avoid such a situation.
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 have noticed the question you raised before submitting. I have already set the initial default value in the parameters, the default parameter value will be the same as the default value generated in previous versions without modifying the parameter value. Therefore, I think the upgrade should be smooth. Is there anything missing from me?
|
||
=== TEST 3: hit | ||
--- request | ||
GET /opentracing |
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.
add error log and status code checks here.
Description
Fixes (#8931)
Checklist