You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by GitBox <gi...@apache.org> on 2022/03/04 07:08:01 UTC
[GitHub] [apisix] zaunist opened a new pull request #6509: fix: backport server-info to 2.10
zaunist opened a new pull request #6509:
URL: https://github.com/apache/apisix/pull/6509
### What this PR does / why we need it:
<!--- Why is this change required? What problem does it solve? -->
<!--- If it fixes an open issue, please link to the issue here. -->
### Pre-submission checklist:
<!--
Please follow the PR manners:
1. Use Draft if the PR is not ready to be reviewed
2. Test is required for the feat/fix PR, unless you have a good reason
3. Doc is required for the feat PR
4. Use a new commit to resolve review instead of `push -f`
5. If you need to resolve merge conflicts after the PR is reviewed, please merge master but do not rebase
6. Use "request review" to notify the reviewer once you have resolved the review
7. Only reviewer can click "Resolve conversation" to mark the reviewer's review resolved
-->
* [x] Did you explain what problem does this PR solve? Or what new features have been added?
* [x] Have you added corresponding test cases?
* [x] Have you modified the corresponding document?
* [x] Is this PR backward compatible? **If it is not backward compatible, please discuss on the [mailing list](https://github.com/apache/apisix/tree/master#community) first**
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] zaunist commented on pull request #6509: change: backport server-info to 2.10
Posted by GitBox <gi...@apache.org>.
zaunist commented on pull request #6509:
URL: https://github.com/apache/apisix/pull/6509#issuecomment-1058929532
cc @tzssangglass @spacewander
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] moonming commented on a change in pull request #6509: change: backport server-info to 2.10
Posted by GitBox <gi...@apache.org>.
moonming commented on a change in pull request #6509:
URL: https://github.com/apache/apisix/pull/6509#discussion_r820323883
##########
File path: t/plugin/server-info.t
##########
@@ -65,58 +79,12 @@ location /t {
ngx.say(json.encode(value))
}
}
---- request
-GET /t
--- response_body eval
-qr/^{"boot_time":\d+,"etcd_version":"[\d\.]+","hostname":"[a-zA-Z\-0-9\.]+","id":[a-zA-Z\-0-9]+,"last_report_time":\d+,"up_time":\d+,"version":"[\d\.]+"}$/
---- no_error_log
-[error]
---- error_log
-timer created to report server info, interval: 60
-
-
-
-=== TEST 2: verify the data integrity after reloading
---- yaml_config
-apisix:
- id: 123456
-plugins:
- - server-info
-plugin_attr:
- server-info:
- report_interval: 60
---- config
-location /t {
- content_by_lua_block {
- local core = require("apisix.core")
- local key = "/data_plane/server_info/" .. core.id.get()
- local res, err = core.etcd.get(key)
- if err ~= nil then
- ngx.status = 500
- ngx.say(err)
- return
- end
-
- local value = res.body.node.value
- if value.up_time >= 2 then
- ngx.say("integral")
- else
- ngx.say("reset")
- end
- }
-}
---- request
-GET /t
---- response_body
-integral
---- no_error_log
-[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\.]+"}$/
Review comment:
So please add more test cases for the added codes
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] membphis commented on a change in pull request #6509: change: backport server-info to 2.10
Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #6509:
URL: https://github.com/apache/apisix/pull/6509#discussion_r821313081
##########
File path: apisix/core/etcd.lua
##########
@@ -360,4 +361,19 @@ function _M.server_version()
end
+function _M.keepalive(id)
+ local etcd_cli, _, err = new()
+ if not etcd_cli then
+ return nil, err
+ end
+
+ local res, err = etcd_cli:keepalive(id)
+ if not res then
+ return nil, err
+ end
+
+ return res, nil
Review comment:
we can create a new issue, fix them later
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] zaunist commented on a change in pull request #6509: change: backport server-info to 2.10
Posted by GitBox <gi...@apache.org>.
zaunist commented on a change in pull request #6509:
URL: https://github.com/apache/apisix/pull/6509#discussion_r820321982
##########
File path: t/plugin/server-info.t
##########
@@ -65,58 +79,12 @@ location /t {
ngx.say(json.encode(value))
}
}
---- request
-GET /t
--- response_body eval
-qr/^{"boot_time":\d+,"etcd_version":"[\d\.]+","hostname":"[a-zA-Z\-0-9\.]+","id":[a-zA-Z\-0-9]+,"last_report_time":\d+,"up_time":\d+,"version":"[\d\.]+"}$/
---- no_error_log
-[error]
---- error_log
-timer created to report server info, interval: 60
-
-
-
-=== TEST 2: verify the data integrity after reloading
---- yaml_config
-apisix:
- id: 123456
-plugins:
- - server-info
-plugin_attr:
- server-info:
- report_interval: 60
---- config
-location /t {
- content_by_lua_block {
- local core = require("apisix.core")
- local key = "/data_plane/server_info/" .. core.id.get()
- local res, err = core.etcd.get(key)
- if err ~= nil then
- ngx.status = 500
- ngx.say(err)
- return
- end
-
- local value = res.body.node.value
- if value.up_time >= 2 then
- ngx.say("integral")
- else
- ngx.say("reset")
- end
- }
-}
---- request
-GET /t
---- response_body
-integral
---- no_error_log
-[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\.]+"}$/
Review comment:
The up_time field has been removed and this test is meaningless here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] zaunist commented on a change in pull request #6509: change: backport server-info to 2.10
Posted by GitBox <gi...@apache.org>.
zaunist commented on a change in pull request #6509:
URL: https://github.com/apache/apisix/pull/6509#discussion_r820321982
##########
File path: t/plugin/server-info.t
##########
@@ -65,58 +79,12 @@ location /t {
ngx.say(json.encode(value))
}
}
---- request
-GET /t
--- response_body eval
-qr/^{"boot_time":\d+,"etcd_version":"[\d\.]+","hostname":"[a-zA-Z\-0-9\.]+","id":[a-zA-Z\-0-9]+,"last_report_time":\d+,"up_time":\d+,"version":"[\d\.]+"}$/
---- no_error_log
-[error]
---- error_log
-timer created to report server info, interval: 60
-
-
-
-=== TEST 2: verify the data integrity after reloading
---- yaml_config
-apisix:
- id: 123456
-plugins:
- - server-info
-plugin_attr:
- server-info:
- report_interval: 60
---- config
-location /t {
- content_by_lua_block {
- local core = require("apisix.core")
- local key = "/data_plane/server_info/" .. core.id.get()
- local res, err = core.etcd.get(key)
- if err ~= nil then
- ngx.status = 500
- ngx.say(err)
- return
- end
-
- local value = res.body.node.value
- if value.up_time >= 2 then
- ngx.say("integral")
- else
- ngx.say("reset")
- end
- }
-}
---- request
-GET /t
---- response_body
-integral
---- no_error_log
-[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\.]+"}$/
Review comment:
The up_time field has been removed and this test is meaningless here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] zaunist commented on pull request #6509: change: backport server-info to 2.10
Posted by GitBox <gi...@apache.org>.
zaunist commented on pull request #6509:
URL: https://github.com/apache/apisix/pull/6509#issuecomment-1061430581
I have create an issue for uniform code style https://github.com/apache/apisix/issues/6541. So we can do it in other PRs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] membphis commented on a change in pull request #6509: change: backport server-info to 2.10
Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #6509:
URL: https://github.com/apache/apisix/pull/6509#discussion_r820186694
##########
File path: apisix/core/etcd.lua
##########
@@ -360,4 +361,19 @@ function _M.server_version()
end
+function _M.keepalive(id)
+ local etcd_cli, _, err = new()
+ if not etcd_cli then
+ return nil, err
+ end
+
+ local res, err = etcd_cli:keepalive(id)
+ if not res then
+ return nil, err
+ end
+
+ return res, nil
Review comment:
the default value is `nil` in this case
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] moonming commented on a change in pull request #6509: change: backport server-info to 2.10
Posted by GitBox <gi...@apache.org>.
moonming commented on a change in pull request #6509:
URL: https://github.com/apache/apisix/pull/6509#discussion_r820218590
##########
File path: t/plugin/server-info.t
##########
@@ -65,58 +79,12 @@ location /t {
ngx.say(json.encode(value))
}
}
---- request
-GET /t
--- response_body eval
-qr/^{"boot_time":\d+,"etcd_version":"[\d\.]+","hostname":"[a-zA-Z\-0-9\.]+","id":[a-zA-Z\-0-9]+,"last_report_time":\d+,"up_time":\d+,"version":"[\d\.]+"}$/
---- no_error_log
-[error]
---- error_log
-timer created to report server info, interval: 60
-
-
-
-=== TEST 2: verify the data integrity after reloading
---- yaml_config
-apisix:
- id: 123456
-plugins:
- - server-info
-plugin_attr:
- server-info:
- report_interval: 60
---- config
-location /t {
- content_by_lua_block {
- local core = require("apisix.core")
- local key = "/data_plane/server_info/" .. core.id.get()
- local res, err = core.etcd.get(key)
- if err ~= nil then
- ngx.status = 500
- ngx.say(err)
- return
- end
-
- local value = res.body.node.value
- if value.up_time >= 2 then
- ngx.say("integral")
- else
- ngx.say("reset")
- end
- }
-}
---- request
-GET /t
---- response_body
-integral
---- no_error_log
-[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\.]+"}$/
Review comment:
I am not sure why you remove test cases when you add many codes?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] zaunist commented on a change in pull request #6509: change: backport server-info to 2.10
Posted by GitBox <gi...@apache.org>.
zaunist commented on a change in pull request #6509:
URL: https://github.com/apache/apisix/pull/6509#discussion_r820328211
##########
File path: t/plugin/server-info.t
##########
@@ -65,58 +79,12 @@ location /t {
ngx.say(json.encode(value))
}
}
---- request
-GET /t
--- response_body eval
-qr/^{"boot_time":\d+,"etcd_version":"[\d\.]+","hostname":"[a-zA-Z\-0-9\.]+","id":[a-zA-Z\-0-9]+,"last_report_time":\d+,"up_time":\d+,"version":"[\d\.]+"}$/
---- no_error_log
-[error]
---- error_log
-timer created to report server info, interval: 60
-
-
-
-=== TEST 2: verify the data integrity after reloading
---- yaml_config
-apisix:
- id: 123456
-plugins:
- - server-info
-plugin_attr:
- server-info:
- report_interval: 60
---- config
-location /t {
- content_by_lua_block {
- local core = require("apisix.core")
- local key = "/data_plane/server_info/" .. core.id.get()
- local res, err = core.etcd.get(key)
- if err ~= nil then
- ngx.status = 500
- ngx.say(err)
- return
- end
-
- local value = res.body.node.value
- if value.up_time >= 2 then
- ngx.say("integral")
- else
- ngx.say("reset")
- end
- }
-}
---- request
-GET /t
---- response_body
-integral
---- no_error_log
-[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\.]+"}$/
Review comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] zaunist commented on a change in pull request #6509: change: backport server-info to 2.10
Posted by GitBox <gi...@apache.org>.
zaunist commented on a change in pull request #6509:
URL: https://github.com/apache/apisix/pull/6509#discussion_r821249236
##########
File path: apisix/core/etcd.lua
##########
@@ -360,4 +361,19 @@ function _M.server_version()
end
+function _M.keepalive(id)
+ local etcd_cli, _, err = new()
+ if not etcd_cli then
+ return nil, err
+ end
+
+ local res, err = etcd_cli:keepalive(id)
+ if not res then
+ return nil, err
+ end
+
+ return res, nil
Review comment:
I checke APISIX repo, other file has the same situation. Example:
- https://github.com/apache/apisix/blob/cb2ba6819f72d75053f8f95aa781975dca2223fd/apisix/balancer/ewma.lua#L93
- https://github.com/apache/apisix/blob/cb2ba6819f72d75053f8f95aa781975dca2223fd/apisix/balancer/ewma.lua#L100
- https://github.com/apache/apisix/blob/cb2ba6819f72d75053f8f95aa781975dca2223fd/apisix/core/schema.lua#L56
- https://github.com/apache/apisix/blob/cb2ba6819f72d75053f8f95aa781975dca2223fd/apisix/core/utils.lua#L137
- https://github.com/apache/apisix/blob/cb2ba6819f72d75053f8f95aa781975dca2223fd/apisix/plugins/basic-auth.lua#L112
- https://github.com/apache/apisix/blob/cb2ba6819f72d75053f8f95aa781975dca2223fd/apisix/plugins/openid-connect.lua#L159
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander merged pull request #6509: change: backport server-info to 2.10
Posted by GitBox <gi...@apache.org>.
spacewander merged pull request #6509:
URL: https://github.com/apache/apisix/pull/6509
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] zaunist commented on a change in pull request #6509: change: backport server-info to 2.10
Posted by GitBox <gi...@apache.org>.
zaunist commented on a change in pull request #6509:
URL: https://github.com/apache/apisix/pull/6509#discussion_r820183194
##########
File path: apisix/core/etcd.lua
##########
@@ -360,4 +361,19 @@ function _M.server_version()
end
+function _M.keepalive(id)
+ local etcd_cli, _, err = new()
+ if not etcd_cli then
+ return nil, err
+ end
+
+ local res, err = etcd_cli:keepalive(id)
+ if not res then
+ return nil, err
+ end
+
+ return res, nil
Review comment:
If I don't return nil, how can I be sure there is no err?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] zaunist commented on a change in pull request #6509: change: backport server-info to 2.10
Posted by GitBox <gi...@apache.org>.
zaunist commented on a change in pull request #6509:
URL: https://github.com/apache/apisix/pull/6509#discussion_r820323414
##########
File path: apisix/core/etcd.lua
##########
@@ -360,4 +361,19 @@ function _M.server_version()
end
+function _M.keepalive(id)
+ local etcd_cli, _, err = new()
+ if not etcd_cli then
+ return nil, err
+ end
+
+ local res, err = etcd_cli:keepalive(id)
+ if not res then
+ return nil, err
+ end
+
+ return res, nil
Review comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] zaunist commented on a change in pull request #6509: change: backport server-info to 2.10
Posted by GitBox <gi...@apache.org>.
zaunist commented on a change in pull request #6509:
URL: https://github.com/apache/apisix/pull/6509#discussion_r821322311
##########
File path: apisix/core/etcd.lua
##########
@@ -360,4 +361,19 @@ function _M.server_version()
end
+function _M.keepalive(id)
+ local etcd_cli, _, err = new()
+ if not etcd_cli then
+ return nil, err
+ end
+
+ local res, err = etcd_cli:keepalive(id)
+ if not res then
+ return nil, err
+ end
+
+ return res, nil
Review comment:
Got it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] membphis commented on a change in pull request #6509: change: backport server-info to 2.10
Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #6509:
URL: https://github.com/apache/apisix/pull/6509#discussion_r820172778
##########
File path: apisix/core/etcd.lua
##########
@@ -360,4 +361,19 @@ function _M.server_version()
end
+function _M.keepalive(id)
+ local etcd_cli, _, err = new()
+ if not etcd_cli then
+ return nil, err
+ end
+
+ local res, err = etcd_cli:keepalive(id)
+ if not res then
+ return nil, err
+ end
+
+ return res, nil
Review comment:
remove `, nil`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] membphis commented on a change in pull request #6509: change: backport server-info to 2.10
Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #6509:
URL: https://github.com/apache/apisix/pull/6509#discussion_r820630797
##########
File path: apisix/core/etcd.lua
##########
@@ -360,4 +361,19 @@ function _M.server_version()
end
+function _M.keepalive(id)
+ local etcd_cli, _, err = new()
+ if not etcd_cli then
+ return nil, err
+ end
+
+ local res, err = etcd_cli:keepalive(id)
+ if not res then
+ return nil, err
+ end
+
+ return res, nil
Review comment:
`, nil` is useless, so we should remove it and keep the code simpler
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org