Signed-off-by: gueFDF <[email protected]> #2288

gueFDF · 2023-03-03T14:32:41Z

What problem does this PR solve?

Issue Number: #2185

Problem Summary:

What is changed and how it works?

What's Changed:
Added some log information, print error code with more meaningful name.

How it Works:
Call the LibCurveErrorName(LIBCURVE_ERROR ret) function to return the specific error information corresponding to the error number.

Side effects(Breaking backward compatibility? Performance regression?):
Basically no side effects.

Check List

Relevant documentation/comments is changed or added
I acknowledge that all my contributions will be made under the project's license

cr-gpt · 2023-03-03T14:32:50Z

copts.bzl

@@ -36,6 +36,7 @@ BASE_FLAGS = [
    "-fno-omit-frame-pointer",
    "-momit-leaf-frame-pointer",
    "-msse4.2",
+    "-faligned-new",
    "-pthread",
 ]



:

The code patch looks correct and should be free of major bugs.

It is recommended that you include a comment above the line of code so that it is easier to understand what the flag is doing.

The "-faligned-new" flag can help improve performance by ensuring that new objects are allocated on aligned memory boundaries. It can also help reduce memory fragmentation.

cr-gpt · 2023-03-03T14:32:54Z

include/client/libcurve_define.h

    LibCurveAioCallBack cb;
-    void*               buf;
+    void *buf;
 } CurveAioContext;

 #endif  // INCLUDE_CLIENT_LIBCURVE_DEFINE_H_


the code review:

The patch looks good, there are no obvious bugs or potential risks. The enum values and typedefs look to be consistent and well structured. The only improvement suggestion I have would be to add comments to the code so future developers can better understand its purpose.

cr-gpt · 2023-03-03T14:33:01Z

src/client/chunkserver_broadcaster.cpp

-}   // namespace client
-}   // namespace curve
+}  // namespace client
+}  // namespace curve


code review:

In @@ -61,8 +61,8 @@ int ChunkServerBroadCaster::BroadCastFileEpoch(, LOG() should use LibCurveErrorName((LIBCURVE_ERROR)ret) to print the error string instead of only ret.

In @@ -73,21 +73,21 @@ int ChunkServerBroadCaster::BroadCastFileEpoch(, LOG() should use LibCurveErrorName((LIBCURVE_ERROR)ret) to print the error string instead of only ret.

The namespace should be changed from curve to curves.

cr-gpt · 2023-03-03T14:33:11Z

src/client/discard_task.cpp

            currentTasks.emplace(kv.first);
        }
    }

-    for (const auto& timerId : currentTasks) {
+    for (const auto &timerId : currentTasks) {
        int ret = bthread_timer_del(timerId);

        if (ret == 0) {


a brief code review:

In the first line, it is better to add a space between "const FInfo" and "*fileInfo" for better readability.

In line 11, it is better to add an error name in the log message to indicate the cause of error.

In line 28, it is better to add a space between "DiscardMetric" and "*metric".

In line 38 and 53, it is better to add a space between "void" and "*arg" for better readability.

In line 41, it is better to add a space between "MetaCache" and "*metaCache".

In line 42, it is better to add a space between "MDSClient" and "*mdsclient".

In line 117, it is better to add a space between "const auto" and "&timerId" for better readability.

cr-gpt · 2023-03-03T14:33:19Z

src/client/lease_executor.cpp

-}   // namespace client
-}   // namespace curve
+}  // namespace client
+}  // namespace curve


our code review.

First, the patch appears to be refactoring the LeaseExecutor class. The Start() and Stop() methods have been changed to accept and return references instead of pointers, which is a good idea as it simplifies the code.

The LeaseValid() function has also been simplified, which is also a good idea.

The CheckNeedUpdateFileInfo() method has also been changed to accept a reference to the FInfo object instead of a pointer. This is a good idea as it simplifies the code.

Overall, this patch looks like it is well written and should not introduce any bugs.

cr-gpt · 2023-03-03T14:33:24Z

src/client/libcbd_libcurve.cpp

@@ -115,4 +115,5 @@ int cbd_libcurve_increase_epoch(const char* filename) {
    return IncreaseEpoch(filename);
 }

+
 }  // extern "C"


with a brief review of the code provided:

The code appears to contain a function called "cbd_libcurve_increase_epoch" that takes in a filename as a parameter. It then calls the IncreaseEpoch function with the provided filename and returns the result. The code looks valid and there is no obvious bug risk.

A potential improvement suggestion would be to add some additional logging or error handling to the function to ensure that any issues are handled appropriately.

cr-gpt · 2023-03-03T14:33:32Z

src/client/libcurve_snapshot.cpp


    if (ret != LIBCURVE_ERROR::OK) {
-        LOG(INFO) << "GetSnapshotSegmentInfo failed, ret = " << ret;
+        LOG(INFO) << "GetSnapshotSegmentInfo failed, ret = " << ret
+                  << LibCurveErrorName((LIBCURVE_ERROR)ret);
        return -ret;
    }



the code review:

The code patch adds an extra log statement to include the name of the error when printing out the error code. This is a good enhancement as it allows for more detailed debugging and troubleshooting.

There is also a log statement added to print out the return value from a failed operation. This is also a good addition as it allows for easier debugging.

The code patch also adds a check for the return value from a function call, which is good practice and helps reduce the risk of bugs.

Overall, the code patch looks good and should not introduce any new bugs.

cr-gpt · 2023-03-03T14:33:39Z

src/client/source_reader.cpp

@@ -175,7 +175,8 @@ int SourceReader::Read(const std::vector<RequestContext*>& reqCtxVec,
                                          UserDataType::IOBuffer);
        if (ret != LIBCURVE_ERROR::OK) {
            LOG(ERROR) << "Read curve failed failed, filename = " << fileName
-                       << ", error = " << ret;
+                       << ", error = " << ret
+                       << LibCurveErrorName((LIBCURVE_ERROR)ret);
            delete curveCombineCtx;
            return -1;
        } else {


with a brief review of the code patch

The code patch seems to be adding an extra line of code, which is aimed at logging more detailed error information by adding the name of the error code to the log message. This should help to easily identify the root cause of any errors that might occur.

From a code quality perspective, this looks good as it does not introduce any new bugs and improves the error logging. However, it is worth considering if there are any areas for improvement. For example, it might be better to use a constant rather than hard-coding the error name in the log message. That way, if the error code changes, the log message will still be accurate.

cr-gpt · 2023-03-03T14:33:46Z

test/integration/snapshotcloneserver/snapshotcloneserver_recover_test.cpp

@@ -674,7 +674,8 @@ class SnapshotCloneServerTest : public ::testing::Test {
        int ret = tracker->GetResult();
        if (ret != LIBCURVE_ERROR::OK) {
            LOG(ERROR) << "RecoverChunk tracker GetResult fail"
-                       << ", ret = " << ret;
+                       << ", ret = " << ret
+                       << LibCurveErrorName((LIBCURVE_ERROR)ret);
            return ret;
        }
        return LIBCURVE_ERROR::OK;


with a brief comment about the code patch above.

It appears that this code patch is aimed at adding logging to the RecoverChunk tracker method, which will now display the LibCurveErrorName associated with the return value of the GetResult call. This should help with debugging in case of errors.

However, there are a few areas of improvement that can be made to this patch:

The logging should be more descriptive, so that it is easier to understand what went wrong.

It may also be beneficial to check if the return value of GetResult is valid before attempting to access the LibCurveErrorName.

Further testing should also be done to ensure that the code works as expected.

gueFDF · 2023-03-03T14:33:49Z

cicheck

cr-gpt bot reviewed Mar 3, 2023

View reviewed changes

gueFDF closed this Mar 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Signed-off-by: gueFDF <[email protected]> #2288

Signed-off-by: gueFDF <[email protected]> #2288

gueFDF commented Mar 3, 2023

cr-gpt bot Mar 3, 2023

cr-gpt bot Mar 3, 2023

cr-gpt bot Mar 3, 2023

cr-gpt bot Mar 3, 2023

cr-gpt bot Mar 3, 2023

cr-gpt bot Mar 3, 2023

cr-gpt bot Mar 3, 2023

cr-gpt bot Mar 3, 2023

cr-gpt bot Mar 3, 2023

gueFDF commented Mar 3, 2023

Signed-off-by: gueFDF <[email protected]> #2288

Signed-off-by: gueFDF <[email protected]> #2288

Conversation

gueFDF commented Mar 3, 2023

What problem does this PR solve?

What is changed and how it works?

Check List

cr-gpt bot Mar 3, 2023

Choose a reason for hiding this comment

cr-gpt bot Mar 3, 2023

Choose a reason for hiding this comment

cr-gpt bot Mar 3, 2023

Choose a reason for hiding this comment

cr-gpt bot Mar 3, 2023

Choose a reason for hiding this comment

cr-gpt bot Mar 3, 2023

Choose a reason for hiding this comment

cr-gpt bot Mar 3, 2023

Choose a reason for hiding this comment

cr-gpt bot Mar 3, 2023

Choose a reason for hiding this comment

cr-gpt bot Mar 3, 2023

Choose a reason for hiding this comment

cr-gpt bot Mar 3, 2023

Choose a reason for hiding this comment

gueFDF commented Mar 3, 2023