fix(log-rotate): skip access log when enable_access_log is set to false by flearc · Pull Request #11310 · apache/apisix

flearc · 2024-05-31T10:26:26Z

Description

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)

…to false

github-actions · 2024-09-17T10:04:58Z

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.

flearc · 2024-09-17T12:50:57Z

@shreemaan-abhishek PTAL

shreemaan-abhishek · 2024-09-25T12:24:29Z

        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 = {}


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

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

shreemaan-abhishek · 2024-09-25T12:24:47Z

    end

-    local new_files = core.table.new(2, 0)
+    local new_files = core.table.new(#files, 0)


seems like unrelated change.

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.

shreemaan-abhishek · 2024-09-25T12:25:07Z



 local function get_log_path_info(file_type)
-    local_conf = core.config.local_conf()


why remove this line?

local_conf is initialized in the init function now.

we should keep this line

the local_conf may change when APISIX is running

and need to update enable_access_log, it may change

enable_access_log = core.table.try_read_attr( local_conf, "nginx_config", "http", "enable_access_log")

thx, first time I learned local_conf can be changed while APISIX running

flearc · 2024-09-29T03:02:31Z

@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 · 2024-10-08T01:56:30Z

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

github-actions · 2024-12-16T10:05:50Z

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 · 2025-01-24T10:05:32Z

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.

flearc · 2025-05-16T10:58:13Z

@Baoyuantop I can't reopen it, maybe need a new PR.

flearc · 2025-05-26T01:49:26Z

@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 needs to be addressed in a separate PR.

Baoyuantop · 2025-06-09T05:55:15Z

Hi @flearc, can you merge the latest master branch?

flearc · 2025-06-09T06:03:43Z

Hi @flearc, can you merge the latest master branch?

@Baoyuantop done

flearc · 2025-06-13T09:45:22Z

@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?

flearc · 2025-06-17T06:52:24Z

Hi @Baoyuantop, the tests have passed. Could you please assign additional reviewers for further review and merge?

membphis · 2025-07-03T08:05:38Z



 local function get_log_path_info(file_type)
-    local_conf = core.config.local_conf()


we should keep this line

the local_conf may change when APISIX is running

membphis · 2025-07-03T08:06:54Z



 local function get_log_path_info(file_type)
-    local_conf = core.config.local_conf()


and need to update enable_access_log, it may change

enable_access_log = core.table.try_read_attr( local_conf, "nginx_config", "http", "enable_access_log")

membphis · 2025-07-04T02:17:48Z

I re-run the failed CI, it seems unrelated to this PR

fix: log-rotate should skip access log when enable_access_log is set …

6a7a5ff

…to false

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 and others added 2 commits June 4, 2024 10:17

Merge branch 'apache:master' into feature/log-rotate

d3f88a7

fix test case

6c07b96

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

fix lint

3afcb7f

shreemaan-abhishek reviewed Jun 10, 2024

View reviewed changes

Comment thread apisix/plugins/log-rotate.lua

Merge branch 'apache:master' into feature/log-rotate

2f63bb2

flearc requested a review from shreemaan-abhishek July 12, 2024 02:23

Merge remote-tracking branch 'origin/master' into feature/log-rotate

7bb61cb

github-actions Bot added the stale label Sep 17, 2024

github-actions Bot removed the stale label Sep 18, 2024

shreemaan-abhishek reviewed Sep 25, 2024

View reviewed changes

fix: core.table.try_read_attr

f34647c

dosubot Bot added the size:M This PR changes 30-99 lines, ignoring generated files. label Sep 29, 2024

flearc requested a review from shreemaan-abhishek September 29, 2024 02:52

shreemaan-abhishek previously approved these changes Oct 3, 2024

View reviewed changes

fix lint ci

fd04395

flearc dismissed shreemaan-abhishek’s stale review via fd04395 October 8, 2024 01:53

flearc requested a review from shreemaan-abhishek October 17, 2024 05:31

github-actions Bot added the stale label Dec 16, 2024

github-actions Bot closed this Jan 24, 2025

Baoyuantop reopened this May 19, 2025

Merge branch 'apache:master' into feature/log-rotate

bd7548f

Merge branch 'apache:master' into feature/log-rotate

e2271da

Baoyuantop added this to ⚡️ Apache APISIX Roadmap Jun 13, 2025

Baoyuantop removed this from ⚡️ Apache APISIX Roadmap Jun 13, 2025

Baoyuantop assigned flearc Jun 13, 2025

Baoyuantop added this to ⚡️ Apache APISIX Roadmap Jun 13, 2025

Baoyuantop moved this to 🏗 In progress in ⚡️ Apache APISIX Roadmap Jun 13, 2025

Baoyuantop moved this from 🏗 In progress to 👀 In review in ⚡️ Apache APISIX Roadmap Jun 13, 2025

Baoyuantop self-requested a review June 17, 2025 08:40

Baoyuantop previously approved these changes Jul 3, 2025

View reviewed changes

Baoyuantop requested review from AlinsRan, Revolyssup, bzp2010, membphis and nic-6443 July 3, 2025 02:08

membphis requested changes Jul 3, 2025

View reviewed changes

fix: init local_conf in init_default_logs

e8f47e9

flearc dismissed Baoyuantop’s stale review via e8f47e9 July 3, 2025 09:37

membphis approved these changes Jul 4, 2025

View reviewed changes

Baoyuantop approved these changes Jul 15, 2025

View reviewed changes

nic-6443 approved these changes Jul 17, 2025

View reviewed changes

Baoyuantop merged commit 81caea1 into apache:master Jul 17, 2025
27 of 31 checks passed

github-project-automation Bot moved this from 👀 In review to ✅ Done in ⚡️ Apache APISIX Roadmap Jul 17, 2025

flearc deleted the feature/log-rotate branch July 17, 2025 10:40



		local function get_log_path_info(file_type)
		local_conf = core.config.local_conf()

Uh oh!

Conversation

flearc commented May 31, 2024

Description

Checklist

Uh oh!

Uh oh!

github-actions Bot commented Sep 17, 2024

Uh oh!

flearc commented Sep 17, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flearc Sep 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flearc commented Sep 29, 2024

Uh oh!

flearc commented Oct 8, 2024

Uh oh!

github-actions Bot commented Dec 16, 2024

Uh oh!

github-actions Bot commented Jan 24, 2025

Uh oh!

flearc commented May 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flearc commented May 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Baoyuantop commented Jun 9, 2025

Uh oh!

flearc commented Jun 9, 2025

Uh oh!

flearc commented Jun 13, 2025

Uh oh!

flearc commented Jun 17, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

membphis commented Jul 4, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

flearc Sep 29, 2024 •

edited

Loading

flearc commented May 16, 2025 •

edited

Loading

flearc commented May 26, 2025 •

edited

Loading