Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/12260 )
Change subject: IMPALA-7985: Port RemoteShutdown() to KRPC. ...................................................................... Patch Set 2: (4 comments) http://gerrit.cloudera.org:8080/#/c/12260/2/be/src/service/client-request-state.cc File be/src/service/client-request-state.cc: http://gerrit.cloudera.org:8080/#/c/12260/2/be/src/service/client-request-state.cc@653 PS2, Line 653: // KRPC relies on resolved IP address, so convert hostname. : IpAddr ip_address; : Status ip_status = HostnameToIpAddr(request.backend.hostname, &ip_address); : if (!ip_status.ok()) { : VLOG(1) << "Could not convert hostname " << request.backend.hostname : << " to ip address, error: " << ip_status.GetDetail(); : return ip_status; : } : TNetworkAddress addr = MakeNetworkAddress(ip_address, port); > In my original (unpublished) prototype I was keeping the original UX where Thanks for the explanation. That's a reasonable scenario in which the backend being shut down may not be found in the backend descriptors. http://gerrit.cloudera.org:8080/#/c/12260/2/be/src/service/client-request-state.cc@692 PS2, Line 692: return Status(Substitute("Rpc to $0 failed with error '$1'. This may be because " : "the port specified is a thrift port, which :shutdown() " : "can no longer use. Make sure the port is a KRPC port.", : TNetworkAddressToString(addr), msg)); : } : return Status(Substitute( : "Rpc to $0 failed with error '$1'.", TNetworkAddressToString(addr), msg)); Not sure if a typical user has any understanding of the difference between Thrift port and KPRC port as they are internal to Impala. May we can simplify the statement to be like the following: string err_string = Substitute("Rpc to $0 failed with error '$1'", TNetworkAddressToString(addr), msg); if (msg.find(...) != string::npos) { err_string.append(" Please make sure the right port is being used or don't specify any port in the :shutdown() command"); } return Status(err_string);} 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); Shouldn't this use a DebugAction instead ? http://gerrit.cloudera.org:8080/#/c/12260/3/be/src/service/control-service.cc@184 PS3, Line 184: FAULT_INJECTION_RPC_DELAY(RPC_REMOTESHUTDOWN); Shouldn't this use a DebugAction instead ? -- 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: 2 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, 29 Jan 2019 23:11:33 +0000 Gerrit-HasComments: Yes
