-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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.
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
fix(file-loger): use no buffering model when open file #7884
fix(file-loger): use no buffering model when open file #7884
Conversation
Avoid error: If the data written to the log file is larger than the default buffer, it will be flushed into the file several times, and other log may be flushed in during this period This bug is introduced in apache#7839
apisix/plugins/file-logger.lua
Outdated
-- No buffering model don't need flush, write to file directly | ||
-- file:flush() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If they are no longer needed, do we need to delete it? BTW, the code lint point this too, there is an empty if branch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@monkeyDluffy6017 I don't think just changing the flush mechanism is a good idea, but it can be an alternative for the users. Also, the default Glibc buffer size should be shown (or the way we can calculate it should be given if the buffer size varies on different OS). |
I don't think we need buffer here, because we write and flush immediately. cc @spacewander |
The number of access logs can be tremendous if the QPS are high, in such a case, the performance will suffer from a serious degradation. Access logs are different from error logs, we definitely need to consider the caching. No buffer can be an option but MUST NOT BE a required one. |
What about using a bigger buffer, like 64KB in Nginx's access_log? IMHO, there still will have an issue that a log > 64KB might be broken. Maybe using |
After rethinking this problem, I have a new opinion: Since the original code doesn't use buffer (it flushes the buffer immediately), I think we can focus on solving the bug first. |
@tokers do you agree with @spacewander ?
|
Got it. |
But we may still open an eye about the performance. |
Description
use no buffering model when open file
Avoid error:
If the data written to the log file is larger than the default buffer, it will be flushed into the file several times, and other log may be flushed in during this period
Fixes #7839
Checklist