Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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: db cannot resume after disk has no space #1765
fix: db cannot resume after disk has no space #1765
Changes from 5 commits
8cbc610
82b2ef1
9433bf3
138269c
613f862
d2ef6ab
00f232b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Based on the provided code patch, here are some observations and suggestions:
The added configurations
least-free-disk-resume-size
andmanually-resume-interval
seem appropriate in terms of functionality. However, it's important to ensure that these values are suitable for your specific use case and environment.For the
least-free-disk-resume-size
configuration, the value of 268435456 bytes corresponds to approximately 256MB, as mentioned in the comment. If this value aligns with your requirements, there doesn't appear to be any bug or issue.Similarly, for the
manually-resume-interval
configuration, the default value of 600 seconds (10 minutes) seems reasonable. Again, make sure it suits your needs.Reviewing the rest of the code patch, there don't appear to be any bugs or risks. However, it's difficult to provide a comprehensive review without seeing the context or the surrounding code.
If you have any specific concerns or further details regarding the code, please provide them for a more thorough review.
Remember to thoroughly test the changes and monitor the system after applying the patch to ensure it behaves as expected in your specific setup.
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.
Here are some observations and suggestions for the code patch:
The commented out section "#compact-interval :" indicates that the configuration option is not being used. If it's necessary, make sure to uncomment it and provide a value.
In the added configurations related to disk usage and resume, ensure that the values provided are appropriate for your system requirements and environment.
The comment "[NOTICE]: least-free-disk-resume-size should not be smaller than write-buffer-size!" highlights an important consideration. Make sure to set the "least-free-disk-resume-size" value accordingly to avoid potential issues.
Review the default values for "min-check-resume-ratio" (0.7) and "manually-resume-interval" (60 seconds) to confirm if they align with your desired behavior.
In the last comment about "window-size," consider reviewing the current value (9000) and determining if increasing it would be beneficial for your specific network latency scenario.
Overall, code reviews often require context and cannot be done solely based on the provided code snippet. It's recommended to review the entire codebase, including dependencies and integrated components, to identify potential bugs, risks, and improvement opportunities more accurately.
Remember to thoroughly test any changes made and consider seeking feedback from other developers familiar with the project for a comprehensive review.
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.
I see it uses
statfs
(deprecated) on the other one. I perfer to usestatvfs
based on https://www.mkssoftware.com/docs/man3/statfs.3.aspThere 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.
@infdahai Could you please help me review the code? #1822 I have made relevant changes to your suggestion
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 上面加上英文注释,体现 ”第一次检验 或者 距离上次 check 时间超过 interval“ 这个 condition
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.
第二,我建议先检测下当前磁盘总体使用情况,整体空闲率低于某个阈值【如 30%】时,才进来执行 check 操作,以减少加锁次数