Support Controller::set_backup_request_policy

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;
     }

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;

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

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;

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());
             }
         }