Alexey Serbin has posted comments on this change. Change subject: KUDU-1580 retry tserver RPC if negotiation times out ......................................................................
Patch Set 6: (4 comments) > (4 comments) > > I'm not sure what the purpose is of NEGOTIATION_TIMED_OUT in the > current patch. The new NEGOTIATION_TIMED_OUT state is to mark an outbound call which timed out on connection negotiation. It's different from timing out on sending data for RPC and awaiting for response. Basically, the idea is to distinguish between errors which happen during connection negotiation and errors that happen while performing a RPC over already established connection. http://gerrit.cloudera.org:8080/#/c/6926/4/src/kudu/rpc/outbound_call.cc File src/kudu/rpc/outbound_call.cc: Line 314: const MonoDelta timeout = controller_->timeout(); > did you mean const& ? not quite so: RpcController::timeout() returns by value, and MonoDelta reference would be the same size as the object itself, so no point to have a reference here. I added 'const' just for readability, to allow the reader to understand this is not going to change in the scope. http://gerrit.cloudera.org:8080/#/c/6926/6/src/kudu/rpc/outbound_call.cc File src/kudu/rpc/outbound_call.cc: Line 330: return state_ == TIMED_OUT; > return state_ == TIMED_OUT || state_ == NEGOTIATION_TIMED_OUT ? I thought about that but was not sure it would work due to the way it's used in CallTransferCallbacks::NotifyTransferFinished I'll give it a second look. http://gerrit.cloudera.org:8080/#/c/6926/4/src/kudu/rpc/outbound_call.h File src/kudu/rpc/outbound_call.h: PS4, Line 124: remove > remote Done http://gerrit.cloudera.org:8080/#/c/6926/6/src/kudu/rpc/rpc_introspection.proto File src/kudu/rpc/rpc_introspection.proto: Line 43: NEGOTIATION_TIMED_OUT = 7; > I don't really understand why these are defined both here and in outbound_c I suspect the idea was to limit dependencies between components and have more flexibility for states of OutboundCall. I just followed the existing practice here. -- To view, visit http://gerrit.cloudera.org:8080/6926 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Icee8bf4978365a23d6627e7bc411b63f53540a3b Gerrit-PatchSet: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
