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

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

fix: Add a layer of logic to check whether the hard disk is executed #1822

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions src/pika_server.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
#include <ifaddrs.h>
#include <netinet/in.h>
#include <sys/resource.h>
#include <sys/statvfs.h>
#include <algorithm>
#include <ctime>
#include <fstream>
Expand Down Expand Up @@ -1519,10 +1520,10 @@ void PikaServer::AutoKeepAliveRSync() {
}

void PikaServer::AutoResumeDB() {
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);
LOG(WARNING) << "statvfs error: " << strerror(errno);
return;
}

Expand All @@ -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 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.

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.

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.

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.

Expand Down