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.

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

Merged
merged 1 commit into from
Jul 27, 2023
Merged

Conversation

Mixficsol
Copy link
Collaborator

fix: #1150

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.
Copy link

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:

  1. 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.

  2. 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.

  3. The updated comments provide clarity on the purpose and default values of the respective configurations.

  4. 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.
Copy link

Choose a reason for hiding this comment

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

Code Review:

  1. The changes you made in lines 219 and 231 to adjust the default values of min-check-resume-ratio and manually-resume-interval seem appropriate. Ensure that these values are tested and confirmed to work correctly in your codebase.

  2. 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, and manually-resume-interval. This will make it easier for other developers to understand the code's intention. Consider adding or improving comments as necessary.

  3. 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.

  4. 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.

@@ -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) {
Copy link
Contributor

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) {
Copy link

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:

  1. 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.

  2. 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.

  3. 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.

  4. 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;
Copy link

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:

  1. An extra condition has been added to the if statement: disk_use_ratio > min_check_resume_ratio.
  2. The original condition last_check_resume_time_.tv_sec == 0 || now.tv_sec - last_check_resume_time_.tv_sec >= interval remains unchanged.
  3. 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, and least_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;
Copy link

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:

  1. Variable Naming: It is unclear from the provided code what disk_use_ratio, min_check_resume_ratio, free_size, and least_free_size represent. It is recommended to use descriptive variable names that clearly convey their purpose.

  2. 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.

  3. 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.

  4. 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;
Copy link

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:

  1. Added Header: A new include directive #include <sys/statvfs.h> is added to include the necessary header file for the statvfs() function.

  2. Data Storage Information: The function AutoResumeDB() retrieves disk information using the statvfs() function instead of statfs(). This change is fine as long as it suits the intended purpose.

  3. 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.

  4. 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.

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);
Copy link
Collaborator

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;
Copy link

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:

  1. It seems that the code is using statfs function to get disk information. However, statvfs is a more modern and preferred alternative. Consider using statvfs consistently throughout the code.

  2. When an error occurs in the call to statvfs, the code logs a warning message with LOG(WARNING). It would be helpful to log the actual error message returned by strerror(errno) along with the warning message. This can provide more specific information about the error.

  3. Within the if condition in the AutoResumeDB function, there is a logical expression that checks both disk_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.

@Mixficsol Mixficsol merged commit 0b61ce7 into OpenAtomFoundation:unstable Jul 27, 2023
@Mixficsol Mixficsol deleted the fix-db branch July 28, 2023 02:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pika 实例硬盘容量满,清理数据后,pika仍然保持错误的状态,需要重启才能恢复
3 participants