nebd-server: drop failed reqeusts' rpc #212

SeanHai · 2021-01-06T12:31:32Z

What problem does this PR solve?

Issue Number: close #xxx

Problem Summary:

What is changed and how it works?

What's Changed:

How it Works:

Side effects(Breaking backward compatibility? Performance regression?):

Check List

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

wu-hanqing · 2021-01-07T11:04:05Z

nebd/src/part2/file_service.cpp

@@ -47,6 +47,9 @@ void NebdFileServiceCallback(NebdServerAioContext* context) {
                response->set_retcode(RetCode::kNoOK);
                LOG(ERROR) << "Read file failed. "
                           << "return code: " << context->ret;
+                // drop the rpc to ensure not return ioerror


这里可以把所有的context->ret<0的分支单独提出来放到一起处理，然后再添加一个配置项，控制error的情况下，是否返回rpc。

wu-hanqing · 2021-01-07T11:04:44Z

nebd/test/part2/file_service_unittest.cpp

@@ -358,18 +358,17 @@ TEST_F(FileServiceTest, CallbackTest) {
    {
        brpc::Controller cntl;
        nebd::client::ReadResponse response;
-        FileServiceTestClosure done;
+        FileServiceTestClosure* done = new FileServiceTestClosure();


这个添加配置后，两种都测试一下。

wu-hanqing · 2021-01-07T11:04:58Z

nebd/test/part2/file_service_unittest.cpp

        context->buf = new butil::IOBuf();
        context->ret = -1;
        NebdFileServiceCallback(context);
-        ASSERT_TRUE(done.IsRunned());


这里可以判断一下ASSERT_FALSE吧

这里可以判断一下ASSERT_FALSE吧

因为 NebdFileServiceCallback(context); 这里就不done释放了

wu-hanqing · 2021-01-11T09:06:14Z

nebd/src/part2/file_service.cpp

@@ -32,71 +32,62 @@ namespace server {

 using nebd::client::RetCode;

+bool returnRpcWhenIoError;


尽量不要把控制参数用全局变量表示。
这里可以在NebdServerAioContext里面加一个变量，表示在io error的情况下，是否返回请求。
在new NebdServerAioContext的地方设置变量。
所以参数保存到NebdFileServiceImpl里面。
然后配置文件的参数，通过构造函数初始化NebdFileServiceImpl里面的参数。

尽量不要把控制参数用全局变量表示。
这里可以在NebdServerAioContext里面加一个变量，表示在io error的情况下，是否返回请求。
在new NebdServerAioContext的地方设置变量。
所以参数保存到NebdFileServiceImpl里面。
然后配置文件的参数，通过构造函数初始化NebdFileServiceImpl里面的参数。

done

wu-hanqing · 2021-01-11T11:14:25Z

nebd/src/part2/util.h

@@ -38,6 +38,8 @@ std::string NebdFileType2Str(NebdFileType type);

 std::string NebdFileStatus2Str(NebdFileStatus status);

+std::string Op2Str(LIBAIO_OP op);


这个实现在哪里？
或者直接对LIBAIO_OP定义一个operator <<

这个实现在哪里？
或者直接对LIBAIO_OP定义一个operator <<

原来就有，在util.cpp中，但是头文件中没声明

wu-hanqing · 2021-01-12T01:58:29Z

nebd/src/part2/file_service.cpp

-            } else {
+    // io error
+    if (context->ret < 0) {
+        LOG(ERROR) << Op2Str(context->op) << " file failed. "


这里可以直接用 LOG(ERROR) << *context吧？
util.h里面声明了std::ostream& operator<<(std::ostream& os, const NebdServerAioContext& c); 函数。
这个函数可以修改一下，加一下context->returnRpcWhenIoError的输出

这里可以直接用 LOG(ERROR) << *context吧？
util.h里面声明了std::ostream& operator<<(std::ostream& os, const NebdServerAioContext& c); 函数。
这个函数可以修改一下，加一下context->returnRpcWhenIoError的输出

done

aspirer

not return io error to upper layer：这个标题改下？IoError应该是个专有名词吧？upper layer应该是指我们的client sdk吧？是不是改成 chunkserver: Never return IoError to client 比较好？

SeanHai · 2021-01-12T05:57:01Z

not return io error to upper layer：这个标题改下？IoError应该是个专有名词吧？upper layer应该是指我们的client sdk吧？是不是改成 chunkserver: Never return IoError to client 比较好？

这里主要是在nebd-server中将发生错误的rpc进行丢弃，目的是让上次IO卡住，等下层恢复后IO可以恢复，改成nebd-server: drop failed reqeusts' rpc

ilixiaocui · 2021-01-20T07:59:33Z

nebd/src/part2/file_service.cpp

-            } else {
+    // io error
+    if (context->ret < 0) {
+        LOG(ERROR) << *context;


*context能正常输出吗

*context能正常输出吗

可以

ilixiaocui · 2021-01-20T08:07:37Z

nebd/src/part2/file_service.cpp

+            return;
+        }
+    } else {
+        switch (context->op) {


这下面都没有处理returnRpcWhenIoError为true的情况

这下面都没有处理returnRpcWhenIoError为true的情况

done

ilixiaocui · 2021-01-20T08:08:04Z

nebd/src/part2/file_service.cpp

+        // don't return rpc
+        if (!context->returnRpcWhenIoError) {
+            // drop the rpc to ensure not return ioerror
+            doneGuard.release();


不return rpc，error日志打一下

不return rpc，error日志打一下

done

ilixiaocui · 2021-01-21T03:34:07Z

nebd/src/part2/file_service.cpp

-                LOG(ERROR) << "Read file failed. "
-                           << "return code: " << context->ret;
+                LOG(ERROR) << "Read file failed: " << *context;
+                // don't return rpc


这里处理重复的很多，是否能封装一下

这里处理重复的很多，是否能封装一下

封装了一个函数SetResponse，主要是在正确返回（context->ret>0）和发生IOError时返回rpc时设置response

ilixiaocui · 2021-01-22T06:26:50Z

nebd/src/part2/file_service.cpp

+    brpc::ClosureGuard doneGuard(context->done);
+
+    // io error
+    if (context->ret < 0) {


if（context->ret < 0 && !context->returnRpcWhenIoError） {
LOG(ERROR) << *context;
doneGuard.release();
delete context->done;
LOG(ERROR) << Op2Str(context->op)
<< " file failed and drop the request rpc.";
} else {
SetResponse(context, RetCode::kOK);
}

想起来还有一个，为什么之前少了一个else分支，但是测试没测出来？

if（context->ret < 0 && !context->returnRpcWhenIoError） {
LOG(ERROR) << *context;
doneGuard.release();
delete context->done;
LOG(ERROR) << Op2Str(context->op)
<< " file failed and drop the request rpc.";
} else {
SetResponse(context, RetCode::kOK);
}

这里主要是两层判断：
1：如果context->ret < 0 说明发生了Error，会打印一条IO Error的错误日志；如果context->ret >= 0 说明没有发生错误直接SetResponse(context, RetCode::kOK);
2：在发生错误的分支里，如果不需要向上层返回Error则丢弃rpc，并打印一条丢弃rpc的错误日志；如果需要向上层返回则设置 SetResponse(context, RetCode::kNoOK);

这样改的话逻辑不对，针对发生错误但是需要向上返回的 response设置了SetResponse(context, RetCode::kOK);

想起来还有一个，为什么之前少了一个else分支，但是测试没测出来？

因为单测里是判断返回的response->retcode为RetCode::kNoOK（-1）,在没有返回的情况下，这个条件也是成立的，这次补充了测试（在发生错误后的返回码设置成别的值，单测能够判断到）

if（context->ret < 0 && !context->returnRpcWhenIoError） {
LOG(ERROR) << *context;
doneGuard.release();
delete context->done;
LOG(ERROR) << Op2Str(context->op)
<< " file failed and drop the request rpc.";
} else if (context->ret < 0) {
} else {
}

ilixiaocui · 2021-01-22T06:29:36Z

nebd/src/part2/file_service.cpp

-                           << "return code: " << context->ret;
-            } else {
+            response->set_retcode(retCode);
+            if (context->ret >= 0) {


？？？漏删了

？？？漏删了

不是，在READ的正常返回除了需要设置response的retcode还需要把读取的数据拷贝进去

add myself as co-mentor for retroactive rules project + add RW project

SeanHai force-pushed the not_return_ioerror branch from d0b9808 to b73f547 Compare January 7, 2021 01:39

wu-hanqing reviewed Jan 7, 2021

View reviewed changes

SeanHai force-pushed the not_return_ioerror branch from b73f547 to c0d147e Compare January 8, 2021 08:12

wu-hanqing reviewed Jan 11, 2021

View reviewed changes

SeanHai force-pushed the not_return_ioerror branch from c0d147e to 517cb08 Compare January 11, 2021 12:01

wu-hanqing reviewed Jan 12, 2021

View reviewed changes

SeanHai force-pushed the not_return_ioerror branch from 517cb08 to 04bf002 Compare January 12, 2021 02:24

aspirer reviewed Jan 12, 2021

View reviewed changes

wu-hanqing approved these changes Jan 12, 2021

View reviewed changes

SeanHai changed the title ~~not return io error to upper layer~~ nebd-server: drop failed reqeusts' rpc Jan 12, 2021

SeanHai force-pushed the not_return_ioerror branch from 04bf002 to 8606ff4 Compare January 12, 2021 06:05

ilixiaocui reviewed Jan 20, 2021

View reviewed changes

SeanHai force-pushed the not_return_ioerror branch 5 times, most recently from c2c27e5 to 1948e6f Compare January 20, 2021 12:14

ilixiaocui reviewed Jan 21, 2021

View reviewed changes

SeanHai force-pushed the not_return_ioerror branch 2 times, most recently from 01bf07b to 8086cb5 Compare January 22, 2021 02:54

ilixiaocui reviewed Jan 22, 2021

View reviewed changes

SeanHai force-pushed the not_return_ioerror branch 8 times, most recently from f994a42 to 8a73898 Compare January 27, 2021 03:40

SeanHai force-pushed the not_return_ioerror branch 2 times, most recently from f91b7df to 50a287e Compare January 27, 2021 12:29

nebd-server: drop failed reqeusts' rpc

1b14769

SeanHai force-pushed the not_return_ioerror branch from 50a287e to 1b14769 Compare January 28, 2021 01:33

ilixiaocui approved these changes Jan 28, 2021

View reviewed changes

ilixiaocui merged commit fef25e0 into opencurve:master Jan 28, 2021

ilixiaocui pushed a commit to ilixiaocui/curve that referenced this pull request Feb 6, 2023

Merge pull request opencurve#212 from cstyan/master

2f80411

add myself as co-mentor for retroactive rules project + add RW project

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

nebd-server: drop failed reqeusts' rpc #212

nebd-server: drop failed reqeusts' rpc #212

SeanHai commented Jan 6, 2021

wu-hanqing Jan 7, 2021

wu-hanqing Jan 7, 2021

wu-hanqing Jan 7, 2021

SeanHai Jan 7, 2021

wu-hanqing Jan 11, 2021

SeanHai Jan 12, 2021

wu-hanqing Jan 11, 2021

SeanHai Jan 11, 2021

wu-hanqing Jan 12, 2021 •

edited

Loading

SeanHai Jan 12, 2021

aspirer left a comment

SeanHai commented Jan 12, 2021

ilixiaocui Jan 20, 2021

SeanHai Jan 20, 2021

ilixiaocui Jan 20, 2021

SeanHai Jan 20, 2021

ilixiaocui Jan 20, 2021

SeanHai Jan 20, 2021

ilixiaocui Jan 21, 2021

SeanHai Jan 22, 2021

ilixiaocui Jan 22, 2021

ilixiaocui Jan 22, 2021

SeanHai Jan 22, 2021

SeanHai Jan 22, 2021

ilixiaocui Jan 22, 2021

ilixiaocui Jan 22, 2021

SeanHai Jan 22, 2021

		@@ -32,71 +32,62 @@ namespace server {

		using nebd::client::RetCode;

		bool returnRpcWhenIoError;

		@@ -38,6 +38,8 @@ std::string NebdFileType2Str(NebdFileType type);

		std::string NebdFileStatus2Str(NebdFileStatus status);

		std::string Op2Str(LIBAIO_OP op);

nebd-server: drop failed reqeusts' rpc #212

nebd-server: drop failed reqeusts' rpc #212

Conversation

SeanHai commented Jan 6, 2021

What problem does this PR solve?

What is changed and how it works?

Check List

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wu-hanqing Jan 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aspirer left a comment

Choose a reason for hiding this comment

SeanHai commented Jan 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wu-hanqing Jan 12, 2021 •

edited

Loading