Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix(log-rotate): skip access log when enable_access_log is set to false #11310

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

fix(log-rotate): skip access log when enable_access_log is set to false #11310

wants to merge 8 commits into from

Conversation

flearc
Copy link
Contributor

@flearc flearc commented May 31, 2024

Description

Fixes #11309

Checklist

  • I have explained the need for this PR and the problem it solves
  • I have explained the changes or the new features added to this PR
  • I have added tests corresponding to this change
  • I have updated the documentation to reflect this change
  • I have verified that this change is backward compatible (If not, please discuss on the APISIX mailing list first)

@flearc flearc changed the title fix(log-rotate): skip access log when enable_access_log is set to false WIP: fix(log-rotate): skip access log when enable_access_log is set to false May 31, 2024
@flearc flearc changed the title WIP: fix(log-rotate): skip access log when enable_access_log is set to false fix(log-rotate): skip access log when enable_access_log is set to false Jun 7, 2024
@github-actions GitHub Actions
Copy link

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 [email protected] list. Thank you for your contributions.

@github-actions github-actions bot added the stale label Sep 17, 2024
@flearc
Copy link
Contributor Author

flearc commented Sep 17, 2024

@shreemaan-abhishek PTAL

@github-actions github-actions bot removed the stale label Sep 18, 2024
shreemaan-abhishek
shreemaan-abhishek reviewed Sep 25, 2024
View reviewed changes
apisix/plugins/log-rotate.lua Outdated Show resolved Hide resolved
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 = {}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why make this change? It seems unrelated to me. If it really is, you should remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same that no need to pre-allocate. It only needs to store one filename when enable_access_log is false.

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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems like unrelated change.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new_files stores the name of access.log and error.log. If enable_access_log is set to false, nginx will not generate access.log file, no need to pre-allocate an array of length 2.

apisix/plugins/log-rotate.lua
@@ -76,7 +77,6 @@ end


local function get_log_path_info(file_type)
local_conf = core.config.local_conf()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why remove this line?

Copy link
Contributor Author

@flearc flearc Sep 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

local_conf is initialized in the init function now.

@dosubot dosubot bot added the size:M This PR changes 30-99 lines, ignoring generated files. label Sep 29, 2024
@flearc
Copy link
Contributor Author

flearc commented Sep 29, 2024

@shreemaan-abhishek Thanks for your advice! I have updated my PR and I would like to explain it again that since we can disable access.log by setting enable_access_log to false, the length of files and new_files is not guaranteed to be 2. So I adjusted the logic here.

@flearc
Copy link
Contributor Author

flearc commented Oct 8, 2024

@shreemaan-abhishek The lint CI has been fixed. The other two test cases' failures seems unrelated. Please review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shreemaan-abhishek shreemaan-abhishek Awaiting requested review from shreemaan-abhishek

At least 3 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
size:M This PR changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

bug: log-rotate plugin should not rotate the access log when the enable_access_log setting is set to false
2 participants
@flearc @shreemaan-abhishek