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

gueFDF · 2023-03-03T15:02:39Z

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

[fix]curve/src Printed some more meaningful log messages. [style] cuerve/src Modify some code specifications.

cr-gpt · 2023-03-03T15:02:50Z

include/client/libcurve_define.h

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

 #endif  // INCLUDE_CLIENT_LIBCURVE_DEFINE_H_


the code review.

The patch is mainly about enum LIBCURVE_ERROR and typedef LIBCURVE_OP and CurveAioContext, the following points are found during the code review:

The code patch is written as expected, each enum constant and typedef statement is followed by a semicolon.

The code patch is consistent with the coding style, all lines are indented by 4 spaces, and the code is well organized.

The code patch is syntactically correct, no compiler errors or warnings will be thrown out when compiling the code.

There is no potential bug risk in the code patch, but there may be potential risks in the logic of the algorithm.

Suggestion: In CurveAioContext, the parameter type of buf should be specified, such as void *buf, char *buf, uint8_t *buf, etc.

cr-gpt · 2023-03-03T15:02:56Z

src/client/chunkserver_broadcaster.cpp

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


with my brief code review:

The code patch appears to have been implemented correctly. It adds a new log message which includes the error code name when an error is encountered. This should make debugging easier and improve user experience. Additionally, it improves readability by adding spaces and line breaks for better organization. There are no obvious risks or bugs that can be seen from the code patch.

cr-gpt · 2023-03-03T15:03:02Z

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


The code looks good and it is following the C++ coding standard.

There are no bugs or errors identified which can be seen in the given code.

Suggestion: In the method RunDiscardTask, consider using a unique_ptr instead of a raw pointer.

Suggestion: In the constructor of DiscardTaskManager, consider using a unique_ptr instead of a raw pointer.

gueFDF · 2023-03-03T15:03:11Z

cicheck

cr-gpt · 2023-03-03T15:03:11Z

src/client/lease_executor.cpp

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


the code review for the above mentioned code patch!

From a quick glance, there doesn't appear to be any major errors in the code patch. It looks like the code is mostly formatting related.

One potential improvement would be to properly format the code and make sure it follows the coding conventions of the project. This will help with readability and maintainability of the code base.

Another suggestion would be to add more comments to the code to explain what each part of the code is doing. This will help other developers better understand the code and make it easier to maintain.

Finally, it's always a good idea to run the code through a static analysis tool to check for any potential bugs or issues. This can help detect any potential problems before they become an issue.

Overall, it looks like the code patch is in good shape and should not pose any major issues.

cr-gpt · 2023-03-03T15:03:19Z

src/client/libcbd_libcurve.cpp

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

+
 }  // extern "C"


with the code review :

The code looks syntactically correct and it should compile without any errors.

The function IncreaseEpoch() is called to increase the epoch for a given filename, however no parameters are passed when calling the function. It is possible that this could cause errors or unexpected behavior if the parameters are not specified correctly.

The code does not appear to check for any errors that may occur when calling the IncreaseEpoch() function. It would be beneficial to add error checking to ensure that the function call was successful.

The code does not appear to perform any validation on the filename parameter before passing it to the IncreaseEpoch() function. It would be beneficial to validate the filename to ensure it is valid before attempting to increase the epoch.

cr-gpt · 2023-03-03T15:03:27Z

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.

In the first block of code, logging should include LibCurveErrorName when logging error code, to make it easier to debug.

In the second block of code, check if the return value of mdsclient.GetFileInfo() is checked. If not, then add an 'if' statement to check the ret value and log a warning message.

In the third block of code, logging should include LibCurveErrorName when logging error code.

Overall, this patch looks good. However, please make sure to check for any other potential bugs that might arise due to the changes made.

cr-gpt · 2023-03-03T15:03:32Z

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 {


the code review.

This code patch looks good overall. It seems to be related to reading in data from a file and logging an error if the read fails. The only improvement I would suggest is to add a comment above the code block explaining what it does and why the change was made. This will help other developers better understand the code.

cr-gpt · 2023-03-03T15:03:38Z

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;


the code review

The code patch seems to be correct, as it adds additional logging to the LOG(ERROR) statement by appending the string representation of the error code returned by the tracker.

However, there are some potential risks that need to be considered. For example, if the LibCurveErrorName function returns an empty string, then the log statement might produce an unexpected result. To address this risk, a check should be added to ensure that the function returns a valid string before printing it.

gueFDF · 2023-03-03T15:09:38Z

cicheck

gueFDF · 2023-03-04T03:47:33Z

cicheck

gueFDF · 2023-03-04T04:27:59Z

cicheck

aspirer · 2023-03-06T01:47:17Z

thanks for your pr. some suggestions:

change the pr title to more meanfull string, such as what are you doing by this pr.
some file code format modifications are not quite appropriate. I have indicated in the relevant files already.

aspirer · 2023-03-06T01:54:33Z

include/client/libcurve_define.h

@@ -29,61 +29,61 @@

 enum LIBCURVE_ERROR {


the original code format looks better.

aspirer · 2023-03-06T01:58:31Z

include/client/libcbd.h

-int cbd_libcurve_increase_epoch(const char* filename);
+int64_t cbd_libcurve_filesize(const char *filename);
+int cbd_libcurve_resize(const char *filename, int64_t size);
+int cbd_libcurve_increase_epoch(const char *filename);


I find that there have two type define styles in curve:
int* p; and int *p;
they are used everywhere, so I think w'd better leave them there, other than change them.
please fix them in other files.

aspirer · 2023-03-06T01:59:31Z

include/client/libcurve_define.h

    // delete the file being cloned
-    DELETE_BEING_CLONED     = 27,
+    DELETE_BEING_CLONED = 27,


the original code style looks better, please do not change them.

aspirer · 2023-03-06T02:01:47Z

src/client/chunkserver_client.cpp

+                       << "id: " << cs.peerID << ", "
+                       << cs.internalAddr.addr_.ip << ":"
+                       << cs.internalAddr.addr_.port;
+            return -::INTERNAL_ERROR;


-LIBCURVE_ERROR::INTERNAL_ERROR;

aspirer · 2023-03-06T02:02:36Z

src/client/lease_executor.cpp

+                             const UserInfo &userinfo, MDSClient *mdsclient,
+                             IOManager4File *iomanager)
+    : fullFileName_(), mdsclient_(mdsclient), userinfo_(userinfo),
+      iomanager_(iomanager), leaseoption_(leaseOpt), leasesession_(),


the original code style is better to me.

aspirer · 2023-03-06T02:03:17Z

src/client/libcbd_ext4.cpp

    return pwrite(fd, buf, length, offset);
 }

-int cbd_ext4_pdiscard(int fd, off_t offset, size_t length) {
+int cbd_ext4_get_LIBCURVE_ERRORpdiscard(int fd, off_t offset, size_t length) {


fix the func name please.

aspirer · 2023-03-06T02:03:56Z

src/client/libcurve_snapshot.cpp

-    LIBCURVE_ERROR ret = mdsclient_.GetFileInfo(filename, userinfo,
-                                                fileInfo, &fEpoch);
+    LIBCURVE_ERROR ret =
+        mdsclient_.GetFileInfo(filename, userinfo, fileInfo, &fEpoch);


the original code style is better to me.

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

5bfe64d

[fix]curve/src Printed some more meaningful log messages. [style] cuerve/src Modify some code specifications.

cr-gpt bot reviewed Mar 3, 2023

View reviewed changes

aspirer reviewed Mar 6, 2023

View reviewed changes

gueFDF closed this by deleting the head repository Mar 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

gueFDF commented 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

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

gueFDF commented Mar 4, 2023

gueFDF commented Mar 4, 2023

aspirer commented Mar 6, 2023

aspirer Mar 6, 2023

aspirer Mar 6, 2023

aspirer Mar 6, 2023

aspirer Mar 6, 2023

aspirer Mar 6, 2023

aspirer Mar 6, 2023

aspirer Mar 6, 2023

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

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

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

gueFDF commented Mar 3, 2023

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

gueFDF commented Mar 4, 2023

gueFDF commented Mar 4, 2023

aspirer commented Mar 6, 2023

aspirer Mar 6, 2023

Choose a reason for hiding this comment

aspirer Mar 6, 2023

Choose a reason for hiding this comment

aspirer Mar 6, 2023

Choose a reason for hiding this comment

aspirer Mar 6, 2023

Choose a reason for hiding this comment

aspirer Mar 6, 2023

Choose a reason for hiding this comment

aspirer Mar 6, 2023

Choose a reason for hiding this comment

aspirer Mar 6, 2023

Choose a reason for hiding this comment