serverglen commented on code in PR #2273:
URL: https://github.com/apache/brpc/pull/2273#discussion_r1246834683


##########
src/brpc/retry_policy.cpp:
##########
@@ -58,4 +54,28 @@ const RetryPolicy* DefaultRetryPolicy() {
     return g_default_policy;
 }
 
+int32_t RpcRetryPolicyWithFixedBackoff::GetBackoffTimeMs(const Controller* 
controller) const {
+    if (controller->retried_count() <= 0) {
+        return 0;
+    }
+    int64_t remaining_rpc_time_ms = (controller->deadline_us() - 
butil::gettimeofday_us()) / 1000;
+    if (remaining_rpc_time_ms < _no_backoff_remaining_rpc_time_ms) {
+        return 0;
+    }
+    return _backoff_time_ms < remaining_rpc_time_ms ? _backoff_time_ms : 0;
+}
+
+int32_t RpcRetryPolicyWithJitteredBackoff::GetBackoffTimeMs(const Controller* 
controller) const {
+    if (controller->retried_count() <= 0) {
+        return 0;
+    }
+    int64_t remaining_rpc_time_ms = controller->deadline_us() / 1000 - 
butil::gettimeofday_ms();

Review Comment:
   这行 和 RpcRetryPolicyWithFixedBackoff::GetBackoffTimeMs 函数中 61 行代码不一样,最好保持统一吧。
   虽然执行结果是一样的



##########
src/brpc/controller.cpp:
##########
@@ -627,33 +632,51 @@ void Controller::OnVersionedRPCReturned(const 
CompletionInfo& info,
         ++_current_call.nretry;
         add_flag(FLAGS_BACKUP_REQUEST);
         return IssueRPC(butil::gettimeofday_us());
-    } else if (_retry_policy ? _retry_policy->DoRetry(this)
-               : DefaultRetryPolicy()->DoRetry(this)) {
-        // The error must come from _current_call because:
-        //  * we intercepted error from _unfinished_call in 
OnVersionedRPCReturned
-        //  * ERPCTIMEDOUT/ECANCELED are not retrying error by default.
-        CHECK_EQ(current_id(), info.id) << "error_code=" << _error_code;
-        if (!SingleServer()) {
-            if (_accessed == NULL) {
-                _accessed = ExcludedServers::Create(
-                    std::min(_max_retry, RETRY_AVOIDANCE));
-                if (NULL == _accessed) {
-                    SetFailed(ENOMEM, "Fail to create ExcludedServers");
-                    goto END_OF_RPC;
+    } else {
+        auto retry_policy = _retry_policy ? _retry_policy : 
DefaultRetryPolicy();
+        if (retry_policy->DoRetry(this)) {
+            // The error must come from _current_call because:
+            //  * we intercepted error from _unfinished_call in 
OnVersionedRPCReturned
+            //  * ERPCTIMEDOUT/ECANCELED are not retrying error by default.
+            CHECK_EQ(current_id(), info.id) << "error_code=" << _error_code;
+            if (!SingleServer()) {
+                if (_accessed == NULL) {
+                    _accessed = ExcludedServers::Create(
+                            std::min(_max_retry, RETRY_AVOIDANCE));
+                    if (NULL == _accessed) {
+                        SetFailed(ENOMEM, "Fail to create ExcludedServers");
+                        goto END_OF_RPC;
+                    }
                 }
+                _accessed->Add(_current_call.peer_id);
             }
-            _accessed->Add(_current_call.peer_id);
-        }
-        _current_call.OnComplete(this, _error_code, info.responded, false);
-        ++_current_call.nretry;
-        // Clear http responses before retrying, otherwise the response may
-        // be mixed with older (and undefined) stuff. This is actually not
-        // done before r32008.
-        if (_http_response) {
-            _http_response->Clear();
+            _current_call.OnComplete(this, _error_code, info.responded, false);
+            ++_current_call.nretry;
+            // Clear http responses before retrying, otherwise the response may
+            // be mixed with older (and undefined) stuff. This is actually not
+            // done before r32008.
+            if (_http_response) {
+                _http_response->Clear();
+            }
+            response_attachment().clear();
+
+            // Retry backoff.
+            bthread::TaskGroup* g = bthread::tls_task_group;
+            if (retry_policy->CanRetryBackoffInPthread() ||
+                (g && !g->is_current_pthread_task())) {
+                int64_t backoff_time_us = retry_policy->GetBackoffTimeMs(this) 
* 1000L;
+                // No need to do retry backoff when the backoff time is longer 
than the remaining rpc time.
+                if (backoff_time_us > 0 &&
+                    backoff_time_us < _deadline_us - butil::gettimeofday_us()) 
{

Review Comment:
   这里应该不用重复判断 backoff_time_us < _deadline_us - butil::gettimeofday_us() 了吧?
   GetBackoffTimeMs 函数里面已经存在对应的逻辑了
   



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to