-
Notifications
You must be signed in to change notification settings - Fork 2.8k
change: backport server-info to 2.10 #6509
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
change: backport server-info to 2.10 #6509
Conversation
apisix/core/etcd.lua
Outdated
| return nil, err | ||
| end | ||
|
|
||
| return res, nil |
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.
remove , nil
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.
If I don't return nil, how can I be sure there is no err?
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 default value is nil in this case
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 checked the other methods in etcd.lua, push, delete, set, etc. all return nil, and I think I'm correct in returning nil here. Shouldn't the code style of a repository be uniform? Even if the default value is nil, it should be explicitly shown in the code, so that the logic of the code is clearer.
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.
, nil is useless, so we should remove it and keep the code simpler
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 checke APISIX repo, other file has the same situation. Example:
-
apisix/apisix/balancer/ewma.lua
Line 93 in cb2ba68
return ewma, nil -
apisix/apisix/balancer/ewma.lua
Line 100 in cb2ba68
return ewma, nil -
Line 56 in cb2ba68
return validator, nil -
Line 137 in cb2ba68
return addr, nil -
apisix/apisix/plugins/basic-auth.lua
Line 112 in cb2ba68
return obj, nil -
apisix/apisix/plugins/openid-connect.lua
Line 159 in cb2ba68
return false, nil, nil
There are more places where there is a similar situation, where nil is returned when err equals nil, do we need to be uniform for those as well?
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.
we can create a new issue, fix them later
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.
Got it.
| [error] | ||
| --- error_log | ||
| timer created to report server info, interval: 60 | ||
| qr/^{"boot_time":\d+,"etcd_version":"[\d\.]+","hostname":"[a-zA-Z\-0-9\.]+","id":[a-zA-Z\-0-9]+,"version":"[\d\.]+"}$/ |
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 am not sure why you remove test cases when you add many codes?
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.
So please add more test cases for the added codes
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.
In this change, we are using keepalive to replace the put operation, and testing keepalive requires verification of revision changes in etcd. However, since test-nginx reloads nginx every time you run a test, this may cause the revision to change, and I have not found a way to verify the keepalive.
membphis
left a comment
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.
LGTM
|
I have create an issue for uniform code style #6541. So we can do it in other PRs. |
What this PR does / why we need it:
Currently, server-info is refactored, so we should backport to 2.10 release. Because 2.10 is a LTS version.
Pre-submission checklist: