Re: [PR] Support fixed and jittered retry backoff policy (brpc)
serverglen merged PR #2273: URL: https://github.com/apache/brpc/pull/2273 -- 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: dev-unsubscr...@brpc.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@brpc.apache.org For additional commands, e-mail: dev-h...@brpc.apache.org
Re: [PR] Support fixed and jittered retry backoff policy (brpc)
serverglen commented on PR #2273: URL: https://github.com/apache/brpc/pull/2273#issuecomment-1620062225 LGTM -- 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: dev-unsubscr...@brpc.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@brpc.apache.org For additional commands, e-mail: dev-h...@brpc.apache.org
Re: [PR] Support fixed and jittered retry backoff policy (brpc)
chenBright commented on code in PR #2273:
URL: https://github.com/apache/brpc/pull/2273#discussion_r1246868770
##
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:
done
##
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:
嗯，统一在controller里判断吧，这是框架的约束，由框架来保证。
另外删除Retry里对重试次数的判断，框架保证了重试时才会调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: dev-unsubscr...@brpc.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@brpc.apache.org
For additional commands, e-mail: dev-h...@brpc.apache.org
Re: [PR] Support fixed and jittered retry backoff policy (brpc)
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: dev-unsubscr...@brpc.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail:
Re: [PR] Support fixed and jittered retry backoff policy (brpc)
wwbmmm commented on PR #2273: URL: https://github.com/apache/brpc/pull/2273#issuecomment-1608583584 LGTM -- 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: dev-unsubscr...@brpc.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@brpc.apache.org For additional commands, e-mail: dev-h...@brpc.apache.org
Re: [PR] Support fixed and jittered retry backoff policy (brpc)
chenBright commented on PR #2273: URL: https://github.com/apache/brpc/pull/2273#issuecomment-1606148862 @wwbmmm 已更新使用文档 -- 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: dev-unsubscr...@brpc.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@brpc.apache.org For additional commands, e-mail: dev-h...@brpc.apache.org
Re: [PR] Support fixed and jittered retry backoff policy (brpc)
chenBright commented on code in PR #2273:
URL: https://github.com/apache/brpc/pull/2273#discussion_r1241195858
##
src/brpc/controller.cpp:
##
@@ -625,33 +630,49 @@ 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 && retry_policy->DoRetry(this)) {
Review Comment:
done
--
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: dev-unsubscr...@brpc.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@brpc.apache.org
For additional commands, e-mail: dev-h...@brpc.apache.org
Re: [PR] Support fixed and jittered retry backoff policy (brpc)
wwbmmm commented on PR #2273: URL: https://github.com/apache/brpc/pull/2273#issuecomment-1605958273 master已修复 macos编译失败，可更新 @chenBright -- 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: dev-unsubscr...@brpc.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@brpc.apache.org For additional commands, e-mail: dev-h...@brpc.apache.org
Re: [PR] Support fixed and jittered retry backoff policy (brpc)
chenBright commented on PR #2273: URL: https://github.com/apache/brpc/pull/2273#issuecomment-1605930611 > Macos编译失败，好像是protobuf的版本发生了变化？ 可能是 -- 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: dev-unsubscr...@brpc.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@brpc.apache.org For additional commands, e-mail: dev-h...@brpc.apache.org
Re: [PR] Support fixed and jittered retry backoff policy (brpc)
wwbmmm commented on PR #2273: URL: https://github.com/apache/brpc/pull/2273#issuecomment-1605911695 Macos编译失败，好像是protobuf的版本发生了变化？ -- 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: dev-unsubscr...@brpc.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@brpc.apache.org For additional commands, e-mail: dev-h...@brpc.apache.org
Re: [PR] Support fixed and jittered retry backoff policy (brpc)
wwbmmm commented on code in PR #2273:
URL: https://github.com/apache/brpc/pull/2273#discussion_r1241070789
##
src/brpc/controller.cpp:
##
@@ -625,33 +630,49 @@ 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 && retry_policy->DoRetry(this)) {
Review Comment:
按原来的代码逻辑，无须判断if (retry_policy)
--
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: dev-unsubscr...@brpc.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@brpc.apache.org
For additional commands, e-mail: dev-h...@brpc.apache.org
Re: [PR] Support fixed and jittered retry backoff policy (brpc)
chenBright commented on PR #2273:
URL: https://github.com/apache/brpc/pull/2273#issuecomment-1597044204
> > > > Hi,
> > > > Since `Retry_backoff_policy` or `Backoff_retry_policy` **IS A**
Retry policy, it is not a good idea that both policies exist in channel,
> > > > ```
> > > > , retry_policy(NULL)
> > > > , retry_backoff_policy(NULL)
> > > > ```
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > > It is better that retry_backoff_policy inherits basic interface in a
manner of :
> > > > ```
> > > > class BackoffRetryPolicy : public RetryPolicy {
> > > > bool DoRetry(const Controller* controller) const {
> > > >// details in backoff policy
> > > > }
> > > > }
> > > > ```
> > >
> > >
> > > @thorneliu Good idea. Maybe These virtual functions can be added to
RetryPolicy with default implementation that the retry backoff feature is
turned off. In a manner of:
> > > ```c++
> > > class RetryPolicy {
> > > public:
> > > virtual ~RetryPolicy();
> > > virtual bool DoRetry(const Controller* controller) const = 0;
> > > virtual int32_t GetBackoffTimeMs(const Controller* controller, int
nretry,
> > > int64_t remaining_rpc_time_ms)
const { return 0; }
> > > virtual bool CanRetryBackoffInPthread() const { return false; }
> > > };
> > > ```
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > > `FixedRetryBackoffPolicy` and `JitteredRetryBackoffPolicy` inherit
`RpcRetryPolicy` which implements the default rpc retry policy.
> >
> >
> > @thorneliu New implementationhas been updated. Could you spare some tine
to review it again and offer any additional input?
>
> Hi, 不好意思没有及时回复
>
> 我觉得最好是RetryPolicy的接口不添加 GetBackoffTimeMs 或CanRetryBackoffInPthread
>
> 我们是否可以做到，
>
> ```
> fooPolicy = new brpc::fooPolicy(arg1, arg2).
> channeloptions.retryPolicy = fooPolicy;
> ```
>
> 然后类似 BackoffTimeMs 这样的细节封装在各自的policy内部。基本的rpcRetryPolicy接口只暴露一个简单的
`DoRetry()` 方法？
https://github.com/apache/brpc/blob/f27fbb4989e1bb2542596049f0263f234b8afb17/src/brpc/controller.cpp#L630-L655
目前来看，好像不能只通过`DoRetry`实现backoff，框架中`DoRetry`的语义是是否要重试，如果是，还有一些工作要做：
``` c++
_current_call.OnComplete(this, _error_code, info.responded, false);
++_current_call.nretry;
```
如果在`DoRetry`中做backoff，这些工作也会被推迟，不太合适，还是做完这些工作再做backoff合适一些吧。而且用户要实现自定义backoff的话，就得需要十分清楚框架此处的实现（例如pthread上做backoff会阻塞pthread、_current_call.nretry还未加1、之前的rpc出现网络错误，socket被SetFailed的时机也推迟等），否则很容易会踩坑。
--
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: dev-unsubscr...@brpc.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@brpc.apache.org
For additional commands, e-mail: dev-h...@brpc.apache.org
Re: [PR] Support fixed and jittered retry backoff policy (brpc)
thorneliu commented on PR #2273:
URL: https://github.com/apache/brpc/pull/2273#issuecomment-1596394405
> > > Hi,
> > > Since `Retry_backoff_policy` or `Backoff_retry_policy` **IS A** Retry
policy, it is not a good idea that both policies exist in channel,
> > > ```
> > > , retry_policy(NULL)
> > > , retry_backoff_policy(NULL)
> > > ```
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > > It is better that retry_backoff_policy inherits basic interface in a
manner of :
> > > ```
> > > class BackoffRetryPolicy : public RetryPolicy {
> > > bool DoRetry(const Controller* controller) const {
> > >// details in backoff policy
> > > }
> > > }
> > > ```
> >
> >
> > @thorneliu Good idea. Maybe These virtual functions can be added to
RetryPolicy with default implementation that the retry backoff feature is
turned off. In a manner of:
> > ```c++
> > class RetryPolicy {
> > public:
> > virtual ~RetryPolicy();
> > virtual bool DoRetry(const Controller* controller) const = 0;
> > virtual int32_t GetBackoffTimeMs(const Controller* controller, int
nretry,
> > int64_t remaining_rpc_time_ms)
const { return 0; }
> > virtual bool CanRetryBackoffInPthread() const { return false; }
> > };
> > ```
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> > `FixedRetryBackoffPolicy` and `JitteredRetryBackoffPolicy` inherit
`RpcRetryPolicy` which implements the default rpc retry policy.
>
> @thorneliu New implementationhas been updated. Could you spare some tine
to review it again and offer any additional input?
Hi, 不好意思没有及时回复
我觉得最好是RetryPolicy的接口不添加 GetBackoffTimeMs 或CanRetryBackoffInPthread
我们是否可以做到，
```
fooPolicy = new brpc::fooPolicy(arg1, arg2).
channeloptions.retryPolicy = fooPolicy;
然后类似 BackoffTimeMs 这样的细节封装在各自的policy内部。基本的rpcRetryPolicy接口只暴露一个简单的
`DoRetry()` 方法？
--
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: dev-unsubscr...@brpc.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@brpc.apache.org
For additional commands, e-mail: dev-h...@brpc.apache.org
Re: [PR] Support fixed and jittered retry backoff policy (brpc)
chenBright closed pull request #2273: Support fixed and jittered retry backoff policy URL: https://github.com/apache/brpc/pull/2273 -- 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: dev-unsubscr...@brpc.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@brpc.apache.org For additional commands, e-mail: dev-h...@brpc.apache.org
Re: [PR] Support fixed and jittered retry backoff policy (brpc)
chenBright commented on PR #2273:
URL: https://github.com/apache/brpc/pull/2273#issuecomment-1595765049
> > Hi,
> > Since `Retry_backoff_policy` or `Backoff_retry_policy` **IS A** Retry
policy, it is not a good idea that both policies exist in channel,
> > ```
> > , retry_policy(NULL)
> > , retry_backoff_policy(NULL)
> > ```
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> > It is better that retry_backoff_policy inherits basic interface in a
manner of :
> > ```
> > class BackoffRetryPolicy : public RetryPolicy {
> > bool DoRetry(const Controller* controller) const {
> >// details in backoff policy
> > }
> > }
> > ```
>
> @thorneliu Good idea. Maybe These virtual functions can be added to
RetryPolicy with default implementation that the retry backoff feature is
turned off. In a manner of:
>
> ```c++
> class RetryPolicy {
> public:
> virtual ~RetryPolicy();
> virtual bool DoRetry(const Controller* controller) const = 0;
> virtual int32_t GetBackoffTimeMs(const Controller* controller, int
nretry,
> int64_t remaining_rpc_time_ms) const
{ return 0; }
> virtual bool CanRetryBackoffInPthread() const { return false; }
> };
> ```
>
> `FixedRetryBackoffPolicy` and `JitteredRetryBackoffPolicy` inherit
`RpcRetryPolicy` which implements the default rpc retry policy.
@thorneliu New implementationhas been updated. Could you spare some tine to
review it again and offer any additional input?
--
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: dev-unsubscr...@brpc.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@brpc.apache.org
For additional commands, e-mail: dev-h...@brpc.apache.org
Re: [PR] Support fixed and jittered retry backoff policy (brpc)
chenBright commented on PR #2273: URL: https://github.com/apache/brpc/pull/2273#issuecomment-1595762431 > @chenBright 麻烦再补充下使用文档说明吧 @wwbmmm 按照 @thorneliu 意见修改了实现，麻烦再看看。没问题，我再补充使用文档。 -- 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: dev-unsubscr...@brpc.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@brpc.apache.org For additional commands, e-mail: dev-h...@brpc.apache.org
Re: [PR] Support fixed and jittered retry backoff policy (brpc)
chenBright commented on PR #2273: URL: https://github.com/apache/brpc/pull/2273#issuecomment-1595643864 > 或者是不是也可以在RetryPolicy里加一些接口 可以的，这样应该合理一些。 -- 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: dev-unsubscr...@brpc.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@brpc.apache.org For additional commands, e-mail: dev-h...@brpc.apache.org
Re: [PR] Support fixed and jittered retry backoff policy (brpc)
chenBright commented on PR #2273:
URL: https://github.com/apache/brpc/pull/2273#issuecomment-1595643720
> Hi,
>
> Since `Retry_backoff_policy` or `Backoff_retry_policy` **IS A** Retry
policy, it is not a good idea that both policies exist in channel,
>
> ```
> , retry_policy(NULL)
> , retry_backoff_policy(NULL)
> ```
>
> It is better that retry_backoff_policy inherits basic interface in a
manner of :
>
> ```
> class BackoffRetryPolicy : public RetryPolicy {
> bool DoRetry(const Controller* controller) const {
>// details in backoff policy
> }
> }
> ```
@thorneliu Good idea. Maybe These virtual functions can be added to
RetryPolicy with default implementation that the retry backoff feature is
turned off. In a manner of:
``` c++
class RetryPolicy {
public:
virtual ~RetryPolicy();
virtual bool DoRetry(const Controller* controller) const = 0;
virtual int32_t GetBackoffTimeMs(const Controller* controller, int
nretry,
int64_t remaining_rpc_time_ms) const {
return 0; }
virtual bool CanRetryBackoffInPthread() const { return false; }
};
```
`FixedRetryBackoffPolicy` and `JitteredRetryBackoffPolicy` inherit
`RpcRetryPolicy` which implements the default rpc retry policy.
--
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: dev-unsubscr...@brpc.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@brpc.apache.org
For additional commands, e-mail: dev-h...@brpc.apache.org
Re: [PR] Support fixed and jittered retry backoff policy (brpc)
lorinlee commented on PR #2273: URL: https://github.com/apache/brpc/pull/2273#issuecomment-1595487767 或者是不是也可以在RetryPolicy里加一些接口 -- 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: dev-unsubscr...@brpc.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@brpc.apache.org For additional commands, e-mail: dev-h...@brpc.apache.org
Re: [PR] Support fixed and jittered retry backoff policy (brpc)
thorneliu commented on PR #2273:
URL: https://github.com/apache/brpc/pull/2273#issuecomment-1590900611
Hi,
Since `Retry_backoff_policy` or 'Backoff_retry_policy' **IS A** Retry
policy, it is not a good idea
that both policy exist in channel,
```
, retry_policy(NULL)
, retry_backoff_policy(NULL)
```
It is better that retry_backoff_policy inherits basic interface in a manner
of :
```
class BackoffRetryPolicy : public RetryPolicy {
bool DoRetry(const Controller* controller) const {
// details in backoff policy
}
}
```
--
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: dev-unsubscr...@brpc.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@brpc.apache.org
For additional commands, e-mail: dev-h...@brpc.apache.org
Re: [PR] Support fixed and jittered retry backoff policy (brpc)
wwbmmm commented on PR #2273: URL: https://github.com/apache/brpc/pull/2273#issuecomment-1590321256 @chenBright 麻烦再补充下使用文档说明吧 -- 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: dev-unsubscr...@brpc.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@brpc.apache.org For additional commands, e-mail: dev-h...@brpc.apache.org
Re: [PR] Support fixed and jittered retry backoff policy (brpc)
chenBright commented on code in PR #2273:
URL: https://github.com/apache/brpc/pull/2273#discussion_r1228335976
##
src/brpc/channel.h:
##
@@ -119,6 +120,17 @@ struct ChannelOptions {
// Default: NULL
const RetryPolicy* retry_policy;
+// Backoff request before every retry. The interface
+// is defined in src/brpc/retry_backoff.h
+// This object is NOT owned by channel and should
+// remain valid when channel is used.
+// Default: NULL
+const RetryBackoffPolicy* retry_backoff_policy;
+// Enable retry backoff in pthread.
+// Note it will block the pthread.
+// Default: false
+bool enable_retry_backoff_in_pthread;
Review Comment:
done
--
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: dev-unsubscr...@brpc.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@brpc.apache.org
For additional commands, e-mail: dev-h...@brpc.apache.org
Re: [PR] Support fixed and jittered retry backoff policy (brpc)
wwbmmm commented on code in PR #2273:
URL: https://github.com/apache/brpc/pull/2273#discussion_r1227486531
##
src/brpc/channel.h:
##
@@ -119,6 +120,17 @@ struct ChannelOptions {
// Default: NULL
const RetryPolicy* retry_policy;
+// Backoff request before every retry. The interface
+// is defined in src/brpc/retry_backoff.h
+// This object is NOT owned by channel and should
+// remain valid when channel is used.
+// Default: NULL
+const RetryBackoffPolicy* retry_backoff_policy;
+// Enable retry backoff in pthread.
+// Note it will block the pthread.
+// Default: false
+bool enable_retry_backoff_in_pthread;
Review Comment:
这个参数，是否可以给RetryBackoffPolicy加一个虚方法来返回？
这样就不用加太多的参数在ChannelOptions里
--
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: dev-unsubscr...@brpc.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@brpc.apache.org
For additional commands, e-mail: dev-h...@brpc.apache.org
Re: [PR] Support fixed and jittered retry backoff policy (brpc)
chenBright commented on code in PR #2273:
URL: https://github.com/apache/brpc/pull/2273#discussion_r1226914968
##
src/brpc/controller.cpp:
##
@@ -1013,6 +1020,18 @@ void Controller::IssueRPC(int64_t start_realtime_us) {
return;
}
+bthread::TaskGroup* g = bthread::tls_task_group;
+// Only bthread task can retry with backoff.
Review Comment:
ChannelOptions增加一个开关，支持在pthread进行重试退避。
没打开开关，但是需要在pthread task中重试退避时，跳过且打印警告日志。
--
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: dev-unsubscr...@brpc.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@brpc.apache.org
For additional commands, e-mail: dev-h...@brpc.apache.org
Re: [PR] Support fixed and jittered retry backoff policy (brpc)
chenBright commented on code in PR #2273:
URL: https://github.com/apache/brpc/pull/2273#discussion_r1226910937
##
src/brpc/retry_backoff_policy.h:
##
@@ -0,0 +1,74 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+
+#ifndef BRPC_RETRY_BACKOFF_POLICY_H
+#define BRPC_RETRY_BACKOFF_POLICY_H
+
+#include "butil/fast_rand.h"
+
+namespace brpc {
+
+// Inherit this class to customize the RPC retry backoff policy.
+class RetryBackoffPolicy {
+public:
+virtual ~RetryBackoffPolicy() = default;
+
+// Returns the backoff time in milliseconds before every retry.
+virtual int32_t get_backoff_time_ms(int nretry, int64_t
reaming_rpc_time_ms) const = 0;
Review Comment:
done
##
src/brpc/retry_backoff_policy.h:
##
@@ -0,0 +1,74 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+
+#ifndef BRPC_RETRY_BACKOFF_POLICY_H
+#define BRPC_RETRY_BACKOFF_POLICY_H
+
+#include "butil/fast_rand.h"
+
+namespace brpc {
+
+// Inherit this class to customize the RPC retry backoff policy.
+class RetryBackoffPolicy {
+public:
+virtual ~RetryBackoffPolicy() = default;
+
+// Returns the backoff time in milliseconds before every retry.
+virtual int32_t get_backoff_time_ms(int nretry, int64_t
reaming_rpc_time_ms) const = 0;
+//
^
+// don't
forget the const modifier
+};
+
+class FixedRetryBackoffPolicy : public RetryBackoffPolicy {
+public:
+FixedRetryBackoffPolicy(int32_t backoff_time_ms, int32_t
no_backoff_remaining_rpc_time_ms)
+: _backoff_time_ms(backoff_time_ms)
+, _no_backoff_remaining_rpc_time_ms(no_backoff_remaining_rpc_time_ms)
{}
+
+int32_t get_backoff_time_ms(int nretry, int64_t reaming_rpc_time_ms) const
override;
Review Comment:
done
--
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: dev-unsubscr...@brpc.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@brpc.apache.org
For additional commands, e-mail: dev-h...@brpc.apache.org
Re: [PR] Support fixed and jittered retry backoff policy (brpc)
chenBright commented on code in PR #2273:
URL: https://github.com/apache/brpc/pull/2273#discussion_r1226911153
##
test/brpc_channel_unittest.cpp:
##
@@ -1810,6 +1811,73 @@ class ChannelTest : public ::testing::Test{
StopAndJoin();
}
+struct TetsRetryBackoff {
Review Comment:
done
##
src/brpc/controller.cpp:
##
@@ -1013,6 +1020,18 @@ void Controller::IssueRPC(int64_t start_realtime_us) {
return;
}
+bthread::TaskGroup* g = bthread::tls_task_group;
+// Only bthread task can retry with backoff.
+if (g && !g->is_current_pthread_task() && _retry_backoff &&
_current_call.nretry > 0) {
+int64_t remaining_rpc_time_us = _deadline_us -
butil::gettimeofday_us();
Review Comment:
done
--
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: dev-unsubscr...@brpc.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@brpc.apache.org
For additional commands, e-mail: dev-h...@brpc.apache.org
Re: [PR] Support fixed and jittered retry backoff policy (brpc)
wwbmmm commented on code in PR #2273:
URL: https://github.com/apache/brpc/pull/2273#discussion_r1226052504
##
src/brpc/retry_backoff_policy.h:
##
@@ -0,0 +1,74 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+
+#ifndef BRPC_RETRY_BACKOFF_POLICY_H
+#define BRPC_RETRY_BACKOFF_POLICY_H
+
+#include "butil/fast_rand.h"
+
+namespace brpc {
+
+// Inherit this class to customize the RPC retry backoff policy.
+class RetryBackoffPolicy {
+public:
+virtual ~RetryBackoffPolicy() = default;
+
+// Returns the backoff time in milliseconds before every retry.
+virtual int32_t get_backoff_time_ms(int nretry, int64_t
reaming_rpc_time_ms) const = 0;
Review Comment:
大写开关的驼峰命名
##
src/brpc/retry_backoff_policy.h:
##
@@ -0,0 +1,74 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+
+#ifndef BRPC_RETRY_BACKOFF_POLICY_H
+#define BRPC_RETRY_BACKOFF_POLICY_H
+
+#include "butil/fast_rand.h"
+
+namespace brpc {
+
+// Inherit this class to customize the RPC retry backoff policy.
+class RetryBackoffPolicy {
+public:
+virtual ~RetryBackoffPolicy() = default;
+
+// Returns the backoff time in milliseconds before every retry.
+virtual int32_t get_backoff_time_ms(int nretry, int64_t
reaming_rpc_time_ms) const = 0;
+//
^
+// don't
forget the const modifier
+};
+
+class FixedRetryBackoffPolicy : public RetryBackoffPolicy {
+public:
+FixedRetryBackoffPolicy(int32_t backoff_time_ms, int32_t
no_backoff_remaining_rpc_time_ms)
+: _backoff_time_ms(backoff_time_ms)
+, _no_backoff_remaining_rpc_time_ms(no_backoff_remaining_rpc_time_ms)
{}
+
+int32_t get_backoff_time_ms(int nretry, int64_t reaming_rpc_time_ms) const
override;
Review Comment:
reaming -> remaining?
##
src/brpc/controller.cpp:
##
@@ -1013,6 +1020,18 @@ void Controller::IssueRPC(int64_t start_realtime_us) {
return;
}
+bthread::TaskGroup* g = bthread::tls_task_group;
+// Only bthread task can retry with backoff.
+if (g && !g->is_current_pthread_task() && _retry_backoff &&
_current_call.nretry > 0) {
+int64_t remaining_rpc_time_us = _deadline_us -
butil::gettimeofday_us();
Review Comment:
backup request也会走到这里，为backup request进行backoff是不是不合适？
把这段逻辑移到OnVersionedRPCReturned里可能更合适
##
test/brpc_channel_unittest.cpp:
##
@@ -1810,6 +1811,73 @@ class ChannelTest : public ::testing::Test{
StopAndJoin();
}
+struct TetsRetryBackoff {
Review Comment:
Tets -> Test?
##
src/brpc/controller.cpp:
##
@@ -1013,6 +1020,18 @@ void Controller::IssueRPC(int64_t start_realtime_us) {
return;
}
+bthread::TaskGroup* g = bthread::tls_task_group;
+// Only bthread task can retry with backoff.
Review Comment:
如果检测到是pthread task，报个错？
--
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: dev-unsubscr...@brpc.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@brpc.apache.org
For additional commands, e-mail: dev-h...@brp
