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

Reopen log file when rotation is detected #127

Merged
merged 1 commit into from
Jun 13, 2022
Merged

Reopen log file when rotation is detected #127

merged 1 commit into from
Jun 13, 2022

Conversation

ashie
Copy link
Collaborator

@ashie ashie commented Jun 9, 2022 •
edited
Loading

This is another solution for #106 to avoid using inotify.

In the previous vesions, there was no way to detect log rotation event when log file was rotated.

It causes that rotated log file was grabbed by DaemonLogger. This was unexpected behavior.

This commit fixes by reopening log file correctly.

NOTE: This PR depends on inotify so only linux was supported.

Fix #106

@ashie ashie mentioned this pull request Jun 9, 2022
Closed
@ashie ashie requested a review from kenhys June 9, 2022 13:14
@ashie ashie force-pushed the reopen-log branch 2 times, most recently from 9b4c0d5 to 0ded7a6 Compare June 10, 2022 08:10
@kenhys
Copy link
Collaborator

kenhys commented Jun 10, 2022

I'll review this PR, just wait for a while.

@ashie ashie force-pushed the reopen-log branch 2 times, most recently from bf05cbe to c2c5266 Compare June 10, 2022 09:36
kenhys
kenhys reviewed Jun 13, 2022
View reviewed changes
Copy link
Collaborator

@kenhys kenhys left a comment

Choose a reason for hiding this comment

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

It may be useful to explain why extending behavior with a module and that intention a bit in the commit message.

kenhys
kenhys approved these changes Jun 13, 2022
View reviewed changes
Copy link
Collaborator

@kenhys kenhys left a comment

Choose a reason for hiding this comment

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

I support this approach, could you solve the remaining concerns?, then it is OK for me.

@ashie
Copy link
Collaborator Author

ashie commented Jun 13, 2022

It may be useful to explain why extending behavior with a module and that intention a bit in the commit message.

Added in 8842081

@ashie
Copy link
Collaborator Author

ashie commented Jun 13, 2022

I've noticed that the current patch isn't enough.
@ino should be also updated when reopen is called directly by applications.

@ashie ashie force-pushed the reopen-log branch 3 times, most recently from 31502fc to e2e8824 Compare June 13, 2022 07:57
@ashie
Copy link
Collaborator Author

ashie commented Jun 13, 2022

I've noticed that the current patch isn't enough. @ino should be also updated when reopen is called directly by applications.

Fixed in e2e8824

@ashie
Reopen log file when rotation is detected
45a830f 
In the previous vesions, there was no way to detect log rotation event when
log file was rotated by external tool (logrotate)

It causes that DaemonLogger continues to write logs to rotated file even even
when it's already renamed or removed, it's not expected behavior.

This commit fixes by reopening log file when inode is changed.

Signed-off-by: Takuro Ashie <[email protected]>
@ashie ashie merged commit ce08201 into master Jun 13, 2022
@ashie ashie deleted the reopen-log branch June 13, 2022 09:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kenhys kenhys kenhys approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ashie @kenhys