You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by "flea1lt (via GitHub)" <gi...@apache.org> on 2024/05/31 10:26:28 UTC
[PR] fix(log-rotate): skip access log when enable_access_log is set to false [apisix]
flea1lt opened a new pull request, #11310:
URL: https://github.com/apache/apisix/pull/11310
### Description
<!-- Please include a summary of the change and which issue is fixed. -->
<!-- Please also include relevant motivation and context. -->
Fixes #11309
### Checklist
- [x] I have explained the need for this PR and the problem it solves
- [x] I have explained the changes or the new features added to this PR
- [x] I have added tests corresponding to this change
- [ ] I have updated the documentation to reflect this change
- [x] I have verified that this change is backward compatible (If not, please discuss on the [APISIX mailing list](https://github.com/apache/apisix/tree/master#community) first)
<!--
Note
1. Mark the PR as draft until it's ready to be reviewed.
2. Always add/update tests for any changes unless you have a good reason.
3. Always update the documentation to reflect the changes made in the PR.
4. Make a new commit to resolve conversations instead of `push -f`.
5. To resolve merge conflicts, merge master instead of rebasing.
6. Use "request review" to notify the reviewer after making changes.
7. Only a reviewer can mark a conversation as resolved.
-->
--
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
Re: [PR] fix(log-rotate): skip access log when enable_access_log is set to false [apisix]
Posted by "flearc (via GitHub)" <gi...@apache.org>.
flearc commented on PR #11310:
URL: https://github.com/apache/apisix/pull/11310#issuecomment-2355673753
@shreemaan-abhishek PTAL
--
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
Re: [PR] fix(log-rotate): skip access log when enable_access_log is set to false [apisix]
Posted by "shreemaan-abhishek (via GitHub)" <gi...@apache.org>.
shreemaan-abhishek commented on code in PR #11310:
URL: https://github.com/apache/apisix/pull/11310#discussion_r1632990139
##########
apisix/plugins/log-rotate.lua:
##########
@@ -189,10 +189,8 @@ end
local function init_default_logs(logs_info, log_type)
local filepath, filename = get_log_path_info(log_type)
logs_info[log_type] = { type = log_type }
- if filename ~= "off" then
Review Comment:
why did you remove this line?
--
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
Re: [PR] fix(log-rotate): skip access log when enable_access_log is set to false [apisix]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #11310: fix(log-rotate): skip access log when enable_access_log is set to false
URL: https://github.com/apache/apisix/pull/11310
--
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
Re: [PR] fix(log-rotate): skip access log when enable_access_log is set to false [apisix]
Posted by "Baoyuantop (via GitHub)" <gi...@apache.org>.
Baoyuantop commented on PR #11310:
URL: https://github.com/apache/apisix/pull/11310#issuecomment-2893515207
Please merge master branch.
--
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
Re: [PR] fix(log-rotate): skip access log when enable_access_log is set to false [apisix]
Posted by "flearc (via GitHub)" <gi...@apache.org>.
flearc commented on code in PR #11310:
URL: https://github.com/apache/apisix/pull/11310#discussion_r1683700742
##########
apisix/plugins/log-rotate.lua:
##########
@@ -189,10 +189,8 @@ end
local function init_default_logs(logs_info, log_type)
local filepath, filename = get_log_path_info(log_type)
logs_info[log_type] = { type = log_type }
- if filename ~= "off" then
Review Comment:
@shreemaan-abhishek Could you please review this PR? Thank you!
--
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
Re: [PR] fix(log-rotate): skip access log when enable_access_log is set to false [apisix]
Posted by "membphis (via GitHub)" <gi...@apache.org>.
membphis commented on PR #11310:
URL: https://github.com/apache/apisix/pull/11310#issuecomment-3034209039
I re-run the failed CI, it seems unrelated to this PR
--
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
Re: [PR] fix(log-rotate): skip access log when enable_access_log is set to false [apisix]
Posted by "membphis (via GitHub)" <gi...@apache.org>.
membphis commented on code in PR #11310:
URL: https://github.com/apache/apisix/pull/11310#discussion_r2182141476
##########
apisix/plugins/log-rotate.lua:
##########
@@ -76,7 +77,6 @@ end
local function get_log_path_info(file_type)
- local_conf = core.config.local_conf()
Review Comment:
and need to update `enable_access_log`, it may change
```lua
enable_access_log = core.table.try_read_attr(
local_conf, "nginx_config", "http", "enable_access_log")
##########
apisix/plugins/log-rotate.lua:
##########
@@ -76,7 +77,6 @@ end
local function get_log_path_info(file_type)
- local_conf = core.config.local_conf()
Review Comment:
we should keep this line
the `local_conf` may change when APISIX is running
--
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
Re: [PR] fix(log-rotate): skip access log when enable_access_log is set to false [apisix]
Posted by "flearc (via GitHub)" <gi...@apache.org>.
flearc commented on code in PR #11310:
URL: https://github.com/apache/apisix/pull/11310#discussion_r2182395402
##########
apisix/plugins/log-rotate.lua:
##########
@@ -76,7 +77,6 @@ end
local function get_log_path_info(file_type)
- local_conf = core.config.local_conf()
Review Comment:
thx, first time I learned `local_conf` can be changed while APISIX running
--
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
Re: [PR] fix(log-rotate): skip access log when enable_access_log is set to false [apisix]
Posted by "flearc (via GitHub)" <gi...@apache.org>.
flearc commented on PR #11310:
URL: https://github.com/apache/apisix/pull/11310#issuecomment-2908289123
@Baoyuantop The failing CI test is not related to the changes in this PR, and I've verified that the same failure exists in the latest several commits on master, it need be fixed.
--
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
Re: [PR] fix(log-rotate): skip access log when enable_access_log is set to false [apisix]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #11310:
URL: https://github.com/apache/apisix/pull/11310#issuecomment-2612136524
This pull request/issue has been closed due to lack of activity. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.
--
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
Re: [PR] fix(log-rotate): skip access log when enable_access_log is set to false [apisix]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #11310:
URL: https://github.com/apache/apisix/pull/11310#issuecomment-2355153488
This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 4 weeks if no further activity occurs. If you think that's incorrect or this pull request should instead be reviewed, please simply write any comment. Even if closed, you can still revive the PR at any time or discuss it on the dev@apisix.apache.org list. Thank you for your contributions.
--
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
Re: [PR] fix(log-rotate): skip access log when enable_access_log is set to false [apisix]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #11310:
URL: https://github.com/apache/apisix/pull/11310#issuecomment-2545142924
This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 4 weeks if no further activity occurs. If you think that's incorrect or this pull request should instead be reviewed, please simply write any comment. Even if closed, you can still revive the PR at any time or discuss it on the dev@apisix.apache.org list. Thank you for your contributions.
--
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
Re: [PR] fix(log-rotate): skip access log when enable_access_log is set to false [apisix]
Posted by "flearc (via GitHub)" <gi...@apache.org>.
flearc commented on PR #11310:
URL: https://github.com/apache/apisix/pull/11310#issuecomment-2954740563
> Hi @flearc, can you merge the latest master branch?
@Baoyuantop done
--
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
Re: [PR] fix(log-rotate): skip access log when enable_access_log is set to false [apisix]
Posted by "Baoyuantop (via GitHub)" <gi...@apache.org>.
Baoyuantop commented on PR #11310:
URL: https://github.com/apache/apisix/pull/11310#issuecomment-2954722249
Hi @flearc, can you merge the latest master branch?
--
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
Re: [PR] fix(log-rotate): skip access log when enable_access_log is set to false [apisix]
Posted by "flearc (via GitHub)" <gi...@apache.org>.
flearc commented on PR #11310:
URL: https://github.com/apache/apisix/pull/11310#issuecomment-2979154509
Hi @Baoyuantop, the tests have passed. Could you please assign additional reviewers for further review and merge?
--
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
Re: [PR] fix(log-rotate): skip access log when enable_access_log is set to false [apisix]
Posted by "flearc (via GitHub)" <gi...@apache.org>.
flearc commented on PR #11310:
URL: https://github.com/apache/apisix/pull/11310#issuecomment-2969768467
@Baoyuantop
The t/plugin/request-id.t test failed in CI, but I couldn’t reproduce it locally. I believe the failure is not related to the changes in this PR. Could you please help check what might have caused 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
Re: [PR] fix(log-rotate): skip access log when enable_access_log is set to false [apisix]
Posted by "flearc (via GitHub)" <gi...@apache.org>.
flearc commented on PR #11310:
URL: https://github.com/apache/apisix/pull/11310#issuecomment-2893477608
@Baoyuantop Two checks are failing, but they don’t appear to be related to this PR.
--
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
Re: [PR] fix(log-rotate): skip access log when enable_access_log is set to false [apisix]
Posted by "shreemaan-abhishek (via GitHub)" <gi...@apache.org>.
shreemaan-abhishek commented on code in PR #11310:
URL: https://github.com/apache/apisix/pull/11310#discussion_r1775129900
##########
apisix/plugins/log-rotate.lua:
##########
@@ -299,9 +303,9 @@ local function rotate()
elseif max_size > 0 then
local access_log_file_size = file_size(default_logs[DEFAULT_ACCESS_LOG_FILENAME].file)
local error_log_file_size = file_size(default_logs[DEFAULT_ERROR_LOG_FILENAME].file)
- local files = core.table.new(2, 0)
+ local files = {}
Review Comment:
why make this change? It seems unrelated to me. If it really is, you should remove it.
##########
apisix/plugins/log-rotate.lua:
##########
@@ -315,6 +319,11 @@ end
function _M.init()
+ if not local_conf then
+ local_conf = core.config.local_conf()
+ end
+ local nginx_config = local_conf and local_conf.nginx_config
Review Comment:
you can use core.table.try_read_attr() function instead. It's much cleaner.
##########
apisix/plugins/log-rotate.lua:
##########
@@ -76,7 +77,6 @@ end
local function get_log_path_info(file_type)
- local_conf = core.config.local_conf()
Review Comment:
why remove this line?
##########
apisix/plugins/log-rotate.lua:
##########
@@ -210,7 +210,7 @@ local function rotate_file(files, now_time, max_kept, timeout)
return
end
- local new_files = core.table.new(2, 0)
+ local new_files = core.table.new(#files, 0)
Review Comment:
seems like unrelated change.
--
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
Re: [PR] fix(log-rotate): skip access log when enable_access_log is set to false [apisix]
Posted by "flea1lt (via GitHub)" <gi...@apache.org>.
flea1lt commented on code in PR #11310:
URL: https://github.com/apache/apisix/pull/11310#discussion_r1637436804
##########
apisix/plugins/log-rotate.lua:
##########
@@ -189,10 +189,8 @@ end
local function init_default_logs(logs_info, log_type)
local filepath, filename = get_log_path_info(log_type)
logs_info[log_type] = { type = log_type }
- if filename ~= "off" then
Review Comment:
Fixed, I remove it for test and forget to restore 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
Re: [PR] fix(log-rotate): skip access log when enable_access_log is set to false [apisix]
Posted by "flearc (via GitHub)" <gi...@apache.org>.
flearc commented on PR #11310:
URL: https://github.com/apache/apisix/pull/11310#issuecomment-2893567931
> Please merge master branch.
done, please rerun CI tests
--
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
Re: [PR] fix(log-rotate): skip access log when enable_access_log is set to false [apisix]
Posted by "Baoyuantop (via GitHub)" <gi...@apache.org>.
Baoyuantop merged PR #11310:
URL: https://github.com/apache/apisix/pull/11310
--
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