-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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: Add a layer of logic to check whether the hard disk is executed #1822
Conversation
conf/pika.conf
Outdated
@@ -231,7 +231,7 @@ slave-priority : 100 | |||
# Manually trying to resume db interval is configured by manually-resume-interval. | |||
# If db has a background error, it will try to manually call resume() to resume db if satisfy the least free disk to resume. | |||
# Its default value is 60 seconds. | |||
#manually-resume-interval : 60 | |||
#manually-resume-interval : 600 | |||
|
|||
# This window-size determines the amount of data that can be transmitted in a single synchronization process. | |||
# [Tip] In the scenario of high network latency. Increasing this size can improve synchronization efficiency. |
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 code patch provided, here are some observations and suggestions:
-
The comment update regarding the default value of
min-check-resume-ratio
from 0.7 to 0.75 seems fine, assuming it reflects the desired behavior. -
The comment update regarding the default value of
manually-resume-interval
from 60 seconds to 600 seconds (10 minutes) seems reasonable if the intention is to increase the interval between manual resume attempts. -
The updated comments provide clarity on the purpose and default values of the respective configurations.
-
It's difficult to assess any potential bugs without having access to the entire codebase. However, from the provided snippet, there don't seem to be any obvious syntax or logic errors.
Overall, this code patch appears to focus on configuration updates, and as long as these changes align with your intended behavior, they should be fine. Remember to also review how these configuration values are used in the code to ensure there are no functional issues introduced by these changes.
conf/pika.conf
Outdated
@@ -231,7 +231,7 @@ slave-priority : 100 | |||
# Manually trying to resume db interval is configured by manually-resume-interval. | |||
# If db has a background error, it will try to manually call resume() to resume db if satisfy the least free disk to resume. | |||
# Its default value is 60 seconds. | |||
#manually-resume-interval : 60 | |||
#manually-resume-interval : 600 | |||
|
|||
# This window-size determines the amount of data that can be transmitted in a single synchronization process. | |||
# [Tip] In the scenario of high network latency. Increasing this size can improve synchronization efficiency. |
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.
Code Review:
-
The changes you made in lines 219 and 231 to adjust the default values of
min-check-resume-ratio
andmanually-resume-interval
seem appropriate. Ensure that these values are tested and confirmed to work correctly in your codebase. -
It would be helpful to provide comments/explanations for the purpose of certain configurations, such as the meaning of
slave-priority
,min-check-resume-ratio
, andmanually-resume-interval
. This will make it easier for other developers to understand the code's intention. Consider adding or improving comments as necessary. -
It is not clear from the provided code patch if there are bug risks or other areas of improvement. For a more comprehensive review, additional code context would be needed.
-
Make sure to perform thorough testing after incorporating these changes to ensure they function as expected and do not introduce any regressions.
Remember that a code review should involve a holistic analysis of the codebase, including its architecture, design, implementation, and overall maintainability.
src/pika_server.cc
Outdated
@@ -1536,7 +1536,7 @@ void PikaServer::AutoResumeDB() { | |||
struct timeval now; | |||
gettimeofday(&now, nullptr); | |||
// first check or time interval between now and last check is larger than variable "interval" | |||
if (last_check_resume_time_.tv_sec == 0 || now.tv_sec - last_check_resume_time_.tv_sec >= interval) { | |||
if (last_check_resume_time_.tv_sec == 0 || now.tv_sec - last_check_resume_time_.tv_sec >= interval || disk_use_ratio > min_check_resume_ratio) { |
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 ((disk_use_ratio > min_check_resume_ratio) && (last_check_resume_time_.tv_sec == 0 || now.tv_sec - last_check_resume_time_.tv_sec >= interval)) {
src/pika_conf.cc
Outdated
@@ -375,7 +375,6 @@ int PikaConf::Load() { | |||
if (write_buffer_size_ <= 0) { | |||
write_buffer_size_ = 268435456; // 256Mb | |||
} | |||
|
|||
// arena_block_size | |||
GetConfInt64Human("arena-block-size", &arena_block_size_); | |||
if (arena_block_size_ <= 0) { |
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.
The code patch you provided contains a small change in the code. The line containing only a single hyphen "-" has been removed. This change removes an empty statement and doesn't have any significant impact on the code.
Overall, the code patch seems fine without any evident bugs. However, it is difficult to provide an extensive review without having access to the complete codebase. Here are some general improvement suggestions:
-
Comment clarity: Add comments that explain the purpose or intention of certain blocks of code or variables. This helps improve code readability and makes it easier for others (including yourself) to understand the code later on.
-
Error handling: Check for and handle any potential errors or exceptions properly. For example, if
GetConfInt64Human()
returns an error or the expected value is not retrieved, consider how the code should handle such situations. -
Magic numbers: Replace hardcoded values like
268435456
with named constants or variables and provide a descriptive comment explaining their significance. This improves code maintainability and allows for easier modifications in the future. -
Consistent coding style: Ensure consistent formatting across the codebase, including indentation, spacing, and the use of braces for code blocks. Consistency enhances code readability and maintainability.
Remember that these suggestions are general and may not cover specific issues or considerations regarding your project. It's always best to follow the guidelines and conventions established by the project or team you're working with.
@@ -1536,7 +1536,7 @@ void PikaServer::AutoResumeDB() { | |||
struct timeval now; | |||
gettimeofday(&now, nullptr); | |||
// first check or time interval between now and last check is larger than variable "interval" | |||
if (last_check_resume_time_.tv_sec == 0 || now.tv_sec - last_check_resume_time_.tv_sec >= interval) { | |||
if (disk_use_ratio > min_check_resume_ratio && (last_check_resume_time_.tv_sec == 0 || now.tv_sec - last_check_resume_time_.tv_sec >= interval)) { | |||
gettimeofday(&last_check_resume_time_, nullptr); | |||
if (disk_use_ratio < min_check_resume_ratio || free_size < least_free_size){ | |||
return; |
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.
The code patch you provided seems to be a modification in the PikaServer::AutoResumeDB()
function. The changes involve an additional condition in the if
statement.
Here's the summary of the code changes:
- An extra condition has been added to the
if
statement:disk_use_ratio > min_check_resume_ratio
. - The original condition
last_check_resume_time_.tv_sec == 0 || now.tv_sec - last_check_resume_time_.tv_sec >= interval
remains unchanged. - If both conditions are satisfied, the code inside the
if
block will execute; otherwise, it will return early from the function.
Code review:
The modified code appears to be checking the disk usage ratio before performing automatic resumption of the database. This suggests that the database resumption is now dependent on both the time interval and the disk usage ratio being above a certain threshold (min_check_resume_ratio
).
It is difficult to provide specific bug risks or improvement suggestions without understanding the larger context of the code and the purpose of this function. However, here are a few general suggestions:
- Ensure that the variables
disk_use_ratio
,min_check_resume_ratio
,interval
,free_size
, andleast_free_size
are properly defined and initialized before this code section. - Verify that the value of
min_check_resume_ratio
is reasonable and appropriate for your application's requirements. - Consider adding comments to explain the purpose and rationale behind these conditions.
- If possible, write unit tests to cover different scenarios and ensure the function behaves as expected.
Remember to review the complete codebase thoroughly, including how these changes interact with other parts of the program, to identify any potential issues or further areas of improvement.
@@ -1536,7 +1536,7 @@ void PikaServer::AutoResumeDB() { | |||
struct timeval now; | |||
gettimeofday(&now, nullptr); | |||
// first check or time interval between now and last check is larger than variable "interval" | |||
if (last_check_resume_time_.tv_sec == 0 || now.tv_sec - last_check_resume_time_.tv_sec >= interval) { | |||
if (disk_use_ratio > min_check_resume_ratio && (last_check_resume_time_.tv_sec == 0 || now.tv_sec - last_check_resume_time_.tv_sec >= interval)) { | |||
gettimeofday(&last_check_resume_time_, nullptr); | |||
if (disk_use_ratio < min_check_resume_ratio || free_size < least_free_size){ | |||
return; |
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.
The provided code patch seems to be a part of a larger codebase, so I can only review the specific changes you've shared.
In terms of code review, here are some observations and suggestions for improvement:
-
Variable Naming: It is unclear from the provided code what
disk_use_ratio
,min_check_resume_ratio
,free_size
, andleast_free_size
represent. It is recommended to use descriptive variable names that clearly convey their purpose. -
Comparison Order: In the condition
(last_check_resume_time_.tv_sec == 0 || now.tv_sec - last_check_resume_time_.tv_sec >= interval)
, it is generally a good practice to maintain consistent comparison order to improve readability. For example, you can move the first part of the condition (last_check_resume_time_.tv_sec == 0
) to the end. -
Commenting: Consider adding comments to clarify the purpose of the code and any assumptions made. This will help future developers (including yourself) understand the intent behind the logic.
-
Error Handling: Ensure that error handling is implemented properly and appropriately in case
gettimeofday()
or other functions encounter errors.
Without additional context and the full codebase, it is difficult to assess the presence of bugs or potential risks. It is always a good practice to carefully test the changes in different scenarios and analyze edge cases before deploying them to production.
Remember, code reviews are best done by individuals who are familiar with the codebase and its requirements in detail.
@@ -1536,7 +1537,7 @@ void PikaServer::AutoResumeDB() { | |||
struct timeval now; | |||
gettimeofday(&now, nullptr); | |||
// first check or time interval between now and last check is larger than variable "interval" | |||
if (last_check_resume_time_.tv_sec == 0 || now.tv_sec - last_check_resume_time_.tv_sec >= interval) { | |||
if (disk_use_ratio > min_check_resume_ratio && (last_check_resume_time_.tv_sec == 0 || now.tv_sec - last_check_resume_time_.tv_sec >= interval)) { | |||
gettimeofday(&last_check_resume_time_, nullptr); | |||
if (disk_use_ratio < min_check_resume_ratio || free_size < least_free_size){ | |||
return; |
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.
From the code patch you provided, here are some observations and suggestions for improvement:
-
Added Header: A new include directive
#include <sys/statvfs.h>
is added to include the necessary header file for thestatvfs()
function. -
Data Storage Information: The function
AutoResumeDB()
retrieves disk information using thestatvfs()
function instead ofstatfs()
. This change is fine as long as it suits the intended purpose. -
Time Interval Check: The condition for checking the time interval has been modified to include an additional check for
disk_use_ratio > min_check_resume_ratio
. It ensures that the function proceeds with checking the resume conditions only if the disk usage ratio exceeds the minimum threshold. This modification introduces a dependency on an undefined variable,disk_use_ratio
, which needs to be defined and initialized before use. -
Code Formatting: There seems to be a closing brace missing at the end of the code snippet. Please ensure all braces are properly matched.
Overall, the code changes appear to be reasonable, but to provide a more accurate and comprehensive review, it would be helpful to have a larger context of the codebase and its requirements.
src/pika_server.cc
Outdated
struct statfs disk_info; | ||
int ret = statfs(g_pika_conf->db_path().c_str(), &disk_info); | ||
struct statvfs disk_info; | ||
int ret = statvfs(g_pika_conf->db_path().c_str(), &disk_info); | ||
if (ret == -1) { | ||
LOG(WARNING) << "statfs error: " << strerror(errno); |
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.
改下
@@ -1536,7 +1537,7 @@ void PikaServer::AutoResumeDB() { | |||
struct timeval now; | |||
gettimeofday(&now, nullptr); | |||
// first check or time interval between now and last check is larger than variable "interval" | |||
if (last_check_resume_time_.tv_sec == 0 || now.tv_sec - last_check_resume_time_.tv_sec >= interval) { | |||
if (disk_use_ratio > min_check_resume_ratio && (last_check_resume_time_.tv_sec == 0 || now.tv_sec - last_check_resume_time_.tv_sec >= interval)) { | |||
gettimeofday(&last_check_resume_time_, nullptr); | |||
if (disk_use_ratio < min_check_resume_ratio || free_size < least_free_size){ | |||
return; |
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.
The code looks fine overall, but there are a few suggestions for improvement:
-
It seems that the code is using
statfs
function to get disk information. However,statvfs
is a more modern and preferred alternative. Consider usingstatvfs
consistently throughout the code. -
When an error occurs in the call to
statvfs
, the code logs a warning message withLOG(WARNING)
. It would be helpful to log the actual error message returned bystrerror(errno)
along with the warning message. This can provide more specific information about the error. -
Within the
if
condition in theAutoResumeDB
function, there is a logical expression that checks bothdisk_use_ratio > min_check_resume_ratio
and the time interval condition. Consider adding parentheses around the time interval condition to ensure correct evaluation of the expressions.
Other than these suggestions, it's challenging to identify any potential bugs or risks without more context or knowledge about the rest of the codebase.
fix: #1150