curvefs: add memcached startup check #2296
Conversation
![cr-gpt[bot]](https://avatars.githubusercontent.com/in/292659?s=60&v=4){kind=small}
| }; | ||
|
|
||
|
|
||
| TEST_F(MemCachedTest, MultiThreadTask) { | ||
| if(!isRunning_) { | ||
| return; | ||
| } | ||
| // prepare data | ||
| std::vector<std::thread> workers; | ||
| std::vector<std::pair<std::string, std::string>> kvstr = { |
There was a problem hiding this comment.
with a brief overview of the code patch. The patch is for MemCachedTest, a testing class that is used to test the functionality of the memcache server. It starts by adding a boolean flag, isRunning_, and sets it to true or false depending on if the server was started correctly. If the server wasn't started correctly, an error message is logged. Then, in the MultiThreadTask test case, the flag is checked before continuing. This ensures that the code does not proceed if the server was not successfully started.
From a risk perspective, there are no major bugs in the code patch. However, I would suggest that the patch is refactored to make the code clearer and easier to read. For example, instead of nesting the if statement in the loop, the check could be done after the loop and the error message could be printed outside of the loop as well. Additionally, more descriptive variable names can be used to help clarify the logic.
|
cicheck |
| }; | ||
|
|
||
|
|
||
| TEST_F(MemCachedTest, MultiThreadTask) { | ||
| if (!isRunning_) { | ||
| return; | ||
| } | ||
| // prepare data | ||
| std::vector<std::thread> workers; | ||
| std::vector<std::pair<std::string, std::string>> kvstr = { |
There was a problem hiding this comment.
the code review:
- Add logging to indicate if memcached server start failed in setup.
- Add a boolean variable to indicate the status of memcache starting, and return immediately when it is false.
- The do-while loop should be changed to while loop since there is no need to run the loop once before testing the condition.
- The LOG(INFO) line should be moved inside the if-block to avoid unnecessary printing when the server fails to start.
- The LOG(ERROR) message should include more useful information such as the errorlog string.
| isRunning_ = ret && retry <= 100; | ||
| if (!isRunning_) { | ||
| LOG(ERROR) << absl::StrCat( | ||
| "=============== memcache start failed, errorlog: ", errorlog); |
There was a problem hiding this comment.
Use ASSERT_EQ checks instead of output.
There was a problem hiding this comment.
if Use ASSERT_EQ, The ci still failed. I use isRunning_ to skip the following test.
| break; | ||
| } | ||
| std::this_thread::sleep_for(std::chrono::milliseconds(100)); | ||
| retry++; | ||
| } while (1); | ||
| ASSERT_TRUE(ret && retry <= 0) | ||
| << absl::StrCat("memcache start failed, errorlog: ", errorlog); | ||
| LOG(INFO) << "=============== memcache start ok"; | ||
| } | ||
|
|
There was a problem hiding this comment.
to do the code review:
- The code looks generally good.
- A new variable "ret" is added and is used in the if statement. It should be initialized to false before it is used.
- An assertion is added to check if the memcache start failed. This is a good practice as it helps to detect errors earlier on.
- The retry number is set to 100, which might be too high. A lower value should be considered.
|
cicheck |
| break; | ||
| } | ||
| std::this_thread::sleep_for(std::chrono::milliseconds(100)); | ||
| retry++; | ||
| } while (1); | ||
| ASSERT_TRUE(ret && retry <= 0) |
| break; | ||
| } | ||
| std::this_thread::sleep_for(std::chrono::milliseconds(100)); | ||
| retry++; | ||
| } while (1); | ||
| ASSERT_TRUE(ret) | ||
| << absl::StrCat("memcache start failed, errorlog: ", errorlog); | ||
| LOG(INFO) << "=============== memcache start ok"; | ||
| } | ||
|
|
There was a problem hiding this comment.
with a brief examination of the code.
This is a test class for memcached server. It appears to loop until a Set command is issued successfully or until the retry count reaches 100. Once the loop completes, an assertion is made that the Set command must have succeeded, and an error message is provided if it did not.
From a code review perspective, there are a few potential issues worth considering:
-
The maximum number of retries should be configurable, rather than hard-coded. This will make it easier to adjust the retry behavior if needed.
-
The assertion should check that the return value of the Set command was true, rather than just checking whether the loop exited successfully. This will ensure that the command actually succeeded, rather than just indicating that it was attempted.
-
A timeout should be implemented to prevent the loop from running indefinitely in the event of a network issue or other unexpected problem.
Overall, this code looks well-structured and should work as intended. However, the improvements suggested above would make the code more robust and reliable.
|
cicheck |
|
It seems that it must fail when |
|
The error is observed as follows:
When we use sudo to execute, the identified user is I have tried following and it's ok. |
Signed-off-by: Ziy1-Tan <[email protected]>
| }; | ||
| manager_.Get(task); | ||
| taskEnvent.Wait(); | ||
| taskEvent.Wait(); | ||
| ASSERT_EQ(0, memcmp(result, kvstr[i].second.c_str(), 4)); | ||
| ASSERT_TRUE(task->res); | ||
| }); |
There was a problem hiding this comment.
with a brief review of the given code patch.
The first change is that the option for starting memcached server is changed from "-u root" to "-uroot", this change can make the command more concise, but we need to verify if it will cause any issue.
The second change is that an assertion is added after calling Set() to ensure the set operation succeeds, this is good practice and can prevent potential bugs in the future.
The third change is that CountDownEvent variable is renamed from taskEnvent to taskEvent, which is a better name that reflects what the variable is used for.
Overall, the code patch looks good, but we should run some tests to make sure the changes don't break existing functionality.
|
cicheck |
What problem does this PR solve?
Issue Number: #2295
Problem Summary:
What is changed and how it works?
What's Changed:
How it Works:
Side effects(Breaking backward compatibility? Performance regression?):
Check List