From e81c8e8d20d6d3f94c80f94c09b4149b28423307 Mon Sep 17 00:00:00 2001 From: Bright Chen Date: Sat, 10 Aug 2024 17:55:54 +0800 Subject: [PATCH] Support Controller::set_backup_request_policy --- src/brpc/channel.cpp | 3 ++- src/brpc/channel.h | 7 +++++-- src/brpc/controller.cpp | 11 +++++++++-- src/brpc/controller.h | 6 +++++- test/brpc_channel_unittest.cpp | 15 +++++---------- 5 files changed, 26 insertions(+), 16 deletions(-) diff --git a/src/brpc/channel.cpp b/src/brpc/channel.cpp index dbeedf878b..c6c9718c50 100644 --- a/src/brpc/channel.cpp +++ b/src/brpc/channel.cpp @@ -496,7 +496,8 @@ void Channel::CallMethod(const google::protobuf::MethodDescriptor* method, // overriding connect_timeout_ms does not make sense, just use the // one in ChannelOptions cntl->_connect_timeout_ms = _options.connect_timeout_ms; - if (cntl->backup_request_ms() == UNSET_MAGIC_NUM) { + if (cntl->backup_request_ms() == UNSET_MAGIC_NUM && + NULL != cntl->_backup_request_policy) { cntl->set_backup_request_ms(_options.backup_request_ms); cntl->_backup_request_policy = _options.backup_request_policy; } diff --git a/src/brpc/channel.h b/src/brpc/channel.h index 304c6114c3..59e4c80230 100644 --- a/src/brpc/channel.h +++ b/src/brpc/channel.h @@ -56,11 +56,12 @@ struct ChannelOptions { int32_t timeout_ms; // Send another request if RPC does not finish after so many milliseconds. - // Overridable by Controller.set_backup_request_ms(). + // Overridable by Controller.set_backup_request_ms() or + // Controller.set_backup_request_policy(). // The request will be sent to a different server by best effort. // If timeout_ms is set and backup_request_ms >= timeout_ms, backup request // will never be sent. - // backup request does NOT imply server-side cancelation. + // backup request does NOT imply server-side cancellation. // Default: -1 (disabled) // Maximum: 0x7fffffff (roughly 30 days) int32_t backup_request_ms; @@ -115,6 +116,8 @@ struct ChannelOptions { // Customize the backup request time and whether to send backup request. // Priority: `backup_request_policy' > `backup_request_ms'. + // Overridable by Controller.set_backup_request_ms() or + // Controller.set_backup_request_policy(). // This object is NOT owned by channel and should remain valid when channel is used. // Default: NULL const BackupRequestPolicy* backup_request_policy; diff --git a/src/brpc/controller.cpp b/src/brpc/controller.cpp index 858432afbb..285638c273 100644 --- a/src/brpc/controller.cpp +++ b/src/brpc/controller.cpp @@ -346,8 +346,13 @@ void Controller::set_backup_request_ms(int64_t timeout_ms) { } int64_t Controller::backup_request_ms() const { - return NULL != _backup_request_policy ? - _backup_request_policy->GetBackupRequestMs() : _backup_request_ms; + int timeout_ms = NULL != _backup_request_policy ? + _backup_request_policy->GetBackupRequestMs() : _backup_request_ms; + if (timeout_ms > 0x7fffffff) { + timeout_ms = 0x7fffffff; + LOG(WARNING) << "backup_request_ms is limited to 0x7fffffff (roughly 24 days)"; + } + return timeout_ms; } void Controller::set_max_retry(int max_retry) { @@ -1324,6 +1329,7 @@ CallId Controller::call_id() { void Controller::SaveClientSettings(ClientSettings* s) const { s->timeout_ms = _timeout_ms; s->backup_request_ms = _backup_request_ms; + s->backup_request_policy = _backup_request_policy; s->max_retry = _max_retry; s->tos = _tos; s->connection_type = _connection_type; @@ -1336,6 +1342,7 @@ void Controller::SaveClientSettings(ClientSettings* s) const { void Controller::ApplyClientSettings(const ClientSettings& s) { set_timeout_ms(s.timeout_ms); set_backup_request_ms(s.backup_request_ms); + set_backup_request_policy(s.backup_request_policy); set_max_retry(s.max_retry); set_type_of_service(s.tos); set_connection_type(s.connection_type); diff --git a/src/brpc/controller.h b/src/brpc/controller.h index ce6100f068..2e160fd099 100644 --- a/src/brpc/controller.h +++ b/src/brpc/controller.h @@ -181,6 +181,9 @@ friend void policy::ProcessThriftRequest(InputMessageBase*); // Set/get the delay to send backup request in milliseconds. Use // ChannelOptions.backup_request_ms on unset. void set_backup_request_ms(int64_t timeout_ms); + void set_backup_request_policy(const BackupRequestPolicy* policy) { + _backup_request_policy = policy; + } int64_t backup_request_ms() const; // Set/get maximum times of retrying. Use ChannelOptions.max_retry on unset. @@ -671,6 +674,7 @@ friend void policy::ProcessThriftRequest(InputMessageBase*); struct ClientSettings { int32_t timeout_ms; int32_t backup_request_ms; + const BackupRequestPolicy* backup_request_policy; int max_retry; int32_t tos; ConnectionType connection_type; @@ -801,7 +805,7 @@ friend void policy::ProcessThriftRequest(InputMessageBase*); int32_t _timeout_ms; int32_t _connect_timeout_ms; int32_t _backup_request_ms; - // Copied from `Channel' which might be destroyed after CallMethod. + // Priority: `_backup_request_policy' > `_backup_request_ms'. const BackupRequestPolicy* _backup_request_policy; // If this rpc call has retry/backup request,this var save the real timeout for current call int64_t _real_timeout_ms; diff --git a/test/brpc_channel_unittest.cpp b/test/brpc_channel_unittest.cpp index 6e12efa98c..dd7285e71d 100644 --- a/test/brpc_channel_unittest.cpp +++ b/test/brpc_channel_unittest.cpp @@ -1979,8 +1979,8 @@ class ChannelTest : public ::testing::Test{ void TestBackupRequestPolicy(bool single_server, bool async, bool short_connection) { ASSERT_EQ(0, StartAccept(_ep)); - for (int i = 0; i < 3; ++i) { - bool backup = i != 1; + for (int i = 0; i < 2; ++i) { + bool backup = i == 0; std::cout << " *** single=" << single_server << " async=" << async << " short=" << short_connection @@ -1996,12 +1996,7 @@ class ChannelTest : public ::testing::Test{ brpc::Controller cntl; req.set_message(__FUNCTION__); - _backup_request_policy.backup = i == 0; - if (i == 2) { - // use `set_backup_request_ms'. - // Although _backup_request_policy.DoBackup return false, it is ignored. - cntl.set_backup_request_ms(10); // 10ms - } + _backup_request_policy.backup = backup; cntl.set_max_retry(RETRY_NUM); cntl.set_timeout_ms(100); // 100ms req.set_sleep_us(50000); // 50ms @@ -2015,11 +2010,11 @@ class ChannelTest : public ::testing::Test{ // Sleep to let `_messenger' detect `Socket' being `SetFailed' const int64_t start_time = butil::gettimeofday_us(); while (_messenger.ConnectionCount() != 0) { - EXPECT_LT(butil::gettimeofday_us(), start_time + 100000L/*100ms*/); + ASSERT_LT(butil::gettimeofday_us(), start_time + 100000L/*100ms*/); bthread_usleep(1000); } } else { - EXPECT_GE(1ul, _messenger.ConnectionCount()); + ASSERT_GE(1ul, _messenger.ConnectionCount()); } }