You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brpc.apache.org by GitBox <gi...@apache.org> on 2022/01/22 15:15:39 UTC
[GitHub] [incubator-brpc] ehds opened a new pull request #1680: fix(input_messenger) client side retry policy
ehds opened a new pull request #1680:
URL: https://github.com/apache/incubator-brpc/pull/1680
*Ptotocol Rule*: Ptotocol in client-side is fixed except `baidu_std`, because of `streaming_rpc` need create by `baidu_std`. ptotocol.
But in current implementation， there is a chance that client will accept message but not match current protocol.
Case:
Client issue two RPC names `A` (use `hulu` protocol) and `B`(use` baidu_std` protocol) to server in the same connection, If A is sent before B, client will set preferred protocol to `baidu_std`, server will accept these two request and response. Then if response of B received by client before A, B will be parsed success, because it is matched with current protocol(`baidu_std`). When parse response of A by current preferred ptotocol `baidu_std` will failed, according to *Ptotocol Rule*, client think this response is `streaming_rpc`, and try other ptotocols, then response of B will be parsed ok.
That's the point: Shall we just retry `streaming_rpc` when parse failed if current protocol is `baidu_std`? instead of trying all other protocols, or other ptotocol mixed with `baidu_std` will accepted successfully.
--
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-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
[GitHub] [incubator-brpc] lorinlee merged pull request #1680: fix(input_messenger) client side retry policy
Posted by GitBox <gi...@apache.org>.
lorinlee merged pull request #1680:
URL: https://github.com/apache/incubator-brpc/pull/1680
--
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-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
[GitHub] [incubator-brpc] lorinlee commented on a change in pull request #1680: fix(input_messenger) client side retry policy
Posted by GitBox <gi...@apache.org>.
lorinlee commented on a change in pull request #1680:
URL: https://github.com/apache/incubator-brpc/pull/1680#discussion_r818742102
##########
File path: src/brpc/input_messenger.cpp
##########
@@ -67,31 +67,42 @@ ParseResult InputMessenger::CutInputMessage(
// selection or by client.
if (preferred >= 0 && preferred <= max_index
&& _handlers[preferred].parse != NULL) {
- ParseResult result =
- _handlers[preferred].parse(&m->_read_buf, m, read_eof, _handlers[preferred].arg);
- if (result.is_ok() ||
- result.error() == PARSE_ERROR_NOT_ENOUGH_DATA) {
- *index = preferred;
- return result;
- } else if (result.error() != PARSE_ERROR_TRY_OTHERS) {
- // Critical error, return directly.
- LOG_IF(ERROR, result.error() == PARSE_ERROR_TOO_BIG_DATA)
- << "A message from " << m->remote_side()
- << "(protocol=" << _handlers[preferred].name
- << ") is bigger than " << FLAGS_max_body_size
- << " bytes, the connection will be closed."
- " Set max_body_size to allow bigger messages";
- return result;
- }
- if (m->CreatedByConnect() &&
- // baidu_std may fall to streaming_rpc
- (ProtocolType)preferred != PROTOCOL_BAIDU_STD) {
- // The protocol is fixed at client-side, no need to try others.
- LOG(ERROR) << "Fail to parse response from " << m->remote_side()
- << " by " << _handlers[preferred].name
- << " at client-side";
- return MakeParseError(PARSE_ERROR_ABSOLUTELY_WRONG);
- }
+ int cur_index = preferred;
+ do {
+ ParseResult result =
+ _handlers[cur_index].parse(&m->_read_buf, m, read_eof, _handlers[cur_index].arg);
+ if (result.is_ok() ||
+ result.error() == PARSE_ERROR_NOT_ENOUGH_DATA) {
+ *index = cur_index;
+ return result;
+ } else if (result.error() != PARSE_ERROR_TRY_OTHERS) {
+ // Critical error, return directly.
+ LOG_IF(ERROR, result.error() == PARSE_ERROR_TOO_BIG_DATA)
+ << "A message from " << m->remote_side()
+ << "(protocol=" << _handlers[cur_index].name
+ << ") is bigger than " << FLAGS_max_body_size
+ << " bytes, the connection will be closed."
+ " Set max_body_size to allow bigger messages";
+ return result;
+ }
+
+ if (m->CreatedByConnect()) {
+ if((ProtocolType)cur_index == PROTOCOL_BAIDU_STD) {
+ // baidu_std may fall to streaming_rpc.
+ cur_index = (int)PROTOCOL_STREAMING_RPC;
+ continue;
+ } else {
+ // The protocol is fixed at client-side, no need to try others.
+ LOG(ERROR) << "Fail to parse response from " << m->remote_side()
+ << " by " << _handlers[preferred].name
+ << " at client-side";
+ return MakeParseError(PARSE_ERROR_ABSOLUTELY_WRONG);
+ }
+ } else {
+ // Try other protocols.
+ break;
+ }
+ } while (1);
Review comment:
这里用while(true)吧，while(1)其实引入了implicit conversion
--
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-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
[GitHub] [incubator-brpc] wwbmmm commented on pull request #1680: fix(input_messenger) client side retry policy
Posted by GitBox <gi...@apache.org>.
wwbmmm commented on pull request #1680:
URL: https://github.com/apache/incubator-brpc/pull/1680#issuecomment-1019706149
@ehds Thanks for you contribution!
--
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-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
[GitHub] [incubator-brpc] ehds commented on a change in pull request #1680: fix(input_messenger) client side retry policy
Posted by GitBox <gi...@apache.org>.
ehds commented on a change in pull request #1680:
URL: https://github.com/apache/incubator-brpc/pull/1680#discussion_r819322897
##########
File path: src/brpc/input_messenger.cpp
##########
@@ -67,31 +67,42 @@ ParseResult InputMessenger::CutInputMessage(
// selection or by client.
if (preferred >= 0 && preferred <= max_index
&& _handlers[preferred].parse != NULL) {
- ParseResult result =
- _handlers[preferred].parse(&m->_read_buf, m, read_eof, _handlers[preferred].arg);
- if (result.is_ok() ||
- result.error() == PARSE_ERROR_NOT_ENOUGH_DATA) {
- *index = preferred;
- return result;
- } else if (result.error() != PARSE_ERROR_TRY_OTHERS) {
- // Critical error, return directly.
- LOG_IF(ERROR, result.error() == PARSE_ERROR_TOO_BIG_DATA)
- << "A message from " << m->remote_side()
- << "(protocol=" << _handlers[preferred].name
- << ") is bigger than " << FLAGS_max_body_size
- << " bytes, the connection will be closed."
- " Set max_body_size to allow bigger messages";
- return result;
- }
- if (m->CreatedByConnect() &&
- // baidu_std may fall to streaming_rpc
- (ProtocolType)preferred != PROTOCOL_BAIDU_STD) {
- // The protocol is fixed at client-side, no need to try others.
- LOG(ERROR) << "Fail to parse response from " << m->remote_side()
- << " by " << _handlers[preferred].name
- << " at client-side";
- return MakeParseError(PARSE_ERROR_ABSOLUTELY_WRONG);
- }
+ int cur_index = preferred;
+ do {
+ ParseResult result =
+ _handlers[cur_index].parse(&m->_read_buf, m, read_eof, _handlers[cur_index].arg);
+ if (result.is_ok() ||
+ result.error() == PARSE_ERROR_NOT_ENOUGH_DATA) {
Review comment:
是的，否则 PROTOCOL_STREAMING_RPC 每次都会重试两次。
--
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-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
[GitHub] [incubator-brpc] lorinlee commented on a change in pull request #1680: fix(input_messenger) client side retry policy
Posted by GitBox <gi...@apache.org>.
lorinlee commented on a change in pull request #1680:
URL: https://github.com/apache/incubator-brpc/pull/1680#discussion_r818740386
##########
File path: src/brpc/input_messenger.cpp
##########
@@ -67,31 +67,42 @@ ParseResult InputMessenger::CutInputMessage(
// selection or by client.
if (preferred >= 0 && preferred <= max_index
&& _handlers[preferred].parse != NULL) {
- ParseResult result =
- _handlers[preferred].parse(&m->_read_buf, m, read_eof, _handlers[preferred].arg);
- if (result.is_ok() ||
- result.error() == PARSE_ERROR_NOT_ENOUGH_DATA) {
- *index = preferred;
- return result;
- } else if (result.error() != PARSE_ERROR_TRY_OTHERS) {
- // Critical error, return directly.
- LOG_IF(ERROR, result.error() == PARSE_ERROR_TOO_BIG_DATA)
- << "A message from " << m->remote_side()
- << "(protocol=" << _handlers[preferred].name
- << ") is bigger than " << FLAGS_max_body_size
- << " bytes, the connection will be closed."
- " Set max_body_size to allow bigger messages";
- return result;
- }
- if (m->CreatedByConnect() &&
- // baidu_std may fall to streaming_rpc
- (ProtocolType)preferred != PROTOCOL_BAIDU_STD) {
- // The protocol is fixed at client-side, no need to try others.
- LOG(ERROR) << "Fail to parse response from " << m->remote_side()
- << " by " << _handlers[preferred].name
- << " at client-side";
- return MakeParseError(PARSE_ERROR_ABSOLUTELY_WRONG);
- }
+ int cur_index = preferred;
+ do {
+ ParseResult result =
+ _handlers[cur_index].parse(&m->_read_buf, m, read_eof, _handlers[cur_index].arg);
+ if (result.is_ok() ||
+ result.error() == PARSE_ERROR_NOT_ENOUGH_DATA) {
Review comment:
这里如果cur_index先是PROTOCOL_BAIDU_STD，后重试PROTOCOL_STREAMING_RPC成功，需要set_preferred_index吧
--
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-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org