Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/12672 )
Change subject: IMPALA-8143: Enhance DoRpcWithRetry(). ...................................................................... Patch Set 2: (5 comments) http://gerrit.cloudera.org:8080/#/c/12672/2/be/src/rpc/rpc-mgr.h File be/src/rpc/rpc-mgr.h: http://gerrit.cloudera.org:8080/#/c/12672/2/be/src/rpc/rpc-mgr.h@29 PS2, Line 29: using kudu::MonoDelta; Don't use 'using' in header files, just fully qualify it everywhere below. http://gerrit.cloudera.org:8080/#/c/12672/2/be/src/rpc/rpc-mgr.h@185 PS2, Line 185: F&& rpc_call Rather than passing in a lambda that performs the call, how would you feel about passing in the proxy object and the function to call, i.e. the way ClientCache::DoRpc() currently works? The motivation for this is that I've written an equivalent DoAsyncRpcWithRetry() function (see https://gerrit.cloudera.org/#/c/12297/), and for the async case we can't take a lambda that calls the rpc because we need to wrap the callback that gets passed to the rpc function in order to simulate recv errors, and it would be nice for the async and sync cases to work the same. Two downsides that I see to this: - It makes the arg list very long, but this is alleviated somewhat by my patch which should prevent callers from having to specify 'error_msg' or 'debug_action' - It prevents patterns like what you did in query-state.cc. That doesn't matter for now though (see my comments in query-state.cc) and I think we can come up with a solution if needed in the future. http://gerrit.cloudera.org:8080/#/c/12672/2/be/src/rpc/rpc-mgr.h@194 PS2, Line 194: typename L There's really only one signature we should be accepting for the log function (i.e. no args and returns void) so I think it might be cleaner just specify the type, something like: typedef boost::function<void()> RpcLogFn; http://gerrit.cloudera.org:8080/#/c/12672/2/be/src/runtime/query-state.cc File be/src/runtime/query-state.cc: http://gerrit.cloudera.org:8080/#/c/12672/2/be/src/runtime/query-state.cc@400 PS2, Line 400: rpc_status = RpcMgr::DoRpcWithRetry(report_exec_status, log_failure, query_ctx_, We don't actually want to retry this rpc like this. Its low impact if the rpc doesn't succeed, because we'll send the missed info with the next report, so we prefer to minimize the number of report rpcs being sent by not retrying the same report. http://gerrit.cloudera.org:8080/#/c/12672/2/be/src/service/control-service.cc File be/src/service/control-service.cc: http://gerrit.cloudera.org:8080/#/c/12672/2/be/src/service/control-service.cc@171 PS2, Line 171: DebugActionNoFail(impala_server->default_query_options_, "RPC_CANCELQUERYFINSTANCES"); I think its probably easier if you leave this alone in this patch and let me handle it in my debug action patch. For example, because this use of 'default_query_options_' is unfortunate. I think the right solution is to add a new flag, say 'krpc_debug_action', and then define a KrpcDebugActionNoFail() and a few other related things which is all stuff that I'll be touching in my patch anyways. -- To view, visit http://gerrit.cloudera.org:8080/12672 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia9693151c35e02235665b3c285a48c585973d390 Gerrit-Change-Number: 12672 Gerrit-PatchSet: 2 Gerrit-Owner: Andrew Sherman <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Thomas Marshall <[email protected]> Gerrit-Comment-Date: Fri, 08 Mar 2019 21:07:37 +0000 Gerrit-HasComments: Yes
