Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/12260 )
Change subject: IMPALA-7985: Port RemoteShutdown() to KRPC. ...................................................................... Patch Set 4: (9 comments) http://gerrit.cloudera.org:8080/#/c/12260/6/be/src/runtime/coordinator-backend-state.cc File be/src/runtime/coordinator-backend-state.cc: http://gerrit.cloudera.org:8080/#/c/12260/6/be/src/runtime/coordinator-backend-state.cc@445 PS6, Line 445: 10.0L 10 should be sufficient. It will be implicitly casted to double. http://gerrit.cloudera.org:8080/#/c/12260/6/be/src/service/client-request-state.cc File be/src/service/client-request-state.cc: http://gerrit.cloudera.org:8080/#/c/12260/6/be/src/service/client-request-state.cc@692 PS6, Line 692: msg.find("RemoteShutdown() RPC failed: Timed out: connection negotiation to") : != string::npos Do you think it makes sense to spill out this message iff "request.__isset.backend && request.backend.port != 0" ? http://gerrit.cloudera.org:8080/#/c/12260/6/be/src/service/client-request-state.cc@695 PS6, Line 695: This may be because the port specified is a thrift port, which" : " :shutdown() can no longer use. Will it be more precise to say as we didn't match the port specified (if any) is actually a thrift port number, right ? "This may be because the port specified is wrong." http://gerrit.cloudera.org:8080/#/c/12260/6/be/src/service/control-service.h File be/src/service/control-service.h: http://gerrit.cloudera.org:8080/#/c/12260/6/be/src/service/control-service.h@71 PS6, Line 71: class ? http://gerrit.cloudera.org:8080/#/c/12260/6/be/src/service/control-service.h@72 PS6, Line 72: class ? http://gerrit.cloudera.org:8080/#/c/12260/4/be/src/service/control-service.h File be/src/service/control-service.h: http://gerrit.cloudera.org:8080/#/c/12260/4/be/src/service/control-service.h@84 PS4, Line 84: MonoDelta krpc_timeout > My experience in the Java world with TimeUnit is that using a type like Mon Fair point. The coding convention in Impala code (e.g. see StartShutdown()) tends to be that we specific time with a suffix of the time unit instead of passing MonoDelta object around. It seems more consistent with the existing code to use timeout_ms or timeout_s instead. http://gerrit.cloudera.org:8080/#/c/12260/4/be/src/service/control-service.h@92 PS4, Line 92: if (!rpc_status.ok()) continue; > I added the TODO, thanks. I didn't add the actual sleep as that was what we Sure, please address those questions in IMPALA-8143. http://gerrit.cloudera.org:8080/#/c/12260/3/be/src/service/control-service.cc File be/src/service/control-service.cc: http://gerrit.cloudera.org:8080/#/c/12260/3/be/src/service/control-service.cc@170 PS3, Line 170: FAULT_INJECTION_RPC_DELAY(RPC_CANCELQUERYFINSTANCES); > OK, how about I promise to do this in IMPALA-8143? I can see some other pro Please add a TODO IMPALA-8143 as a reminder. http://gerrit.cloudera.org:8080/#/c/12260/6/be/src/service/control-service.cc File be/src/service/control-service.cc: http://gerrit.cloudera.org:8080/#/c/12260/6/be/src/service/control-service.cc@98 PS6, Line 98: kudu::rpc::RpcContext Not your change but can you please replace "kudu::rpc::RpcContext" to RpcContext given the "use kudu::rpc::RpcContext" above ? -- To view, visit http://gerrit.cloudera.org:8080/12260 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4fd00ee4e638f5e71e27893162fd65501ef9e74e Gerrit-Change-Number: 12260 Gerrit-PatchSet: 4 Gerrit-Owner: Andrew Sherman <asher...@cloudera.com> Gerrit-Reviewer: Andrew Sherman <asher...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Thomas Marshall <thomasmarsh...@cmu.edu> Gerrit-Comment-Date: Tue, 05 Feb 2019 19:40:34 +0000 Gerrit-HasComments: Yes
