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: (6 comments) 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@79 PS4, Line 79: /// Retry the Rpc 'rpc_call' up to 'times_to_try' times. May also want to mention each RPC has a timeout of 'timeout' in some time units. Also, there is no sleeping between retries. http://gerrit.cloudera.org:8080/#/c/12260/4/be/src/service/control-service.h@84 PS4, Line 84: MonoDelta krpc_timeout Instead of passing the MonoDelta object itself, how about just defining this as timeout in millisecond ? http://gerrit.cloudera.org:8080/#/c/12260/4/be/src/service/control-service.h@85 PS4, Line 85: DCHECK(times_to_try > 0); DCHECK_GT() http://gerrit.cloudera.org:8080/#/c/12260/4/be/src/service/control-service.h@92 PS4, Line 92: if (!rpc_status.ok()) continue; Please add a TODO about sleeping between retries in case the server is overloaded. Honestly though, I don't see why we cannot add a sleep time between retries in this patch if the remote server's service queue fills up. Otherwise, the retry loop won't be too effective as we burst sending the same RPCs in such a short period of time. 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); > These tests are setting delays on server-side commands so as to test RPC ti This can easily be replaced by DebugAction. Please see https://github.com/apache/impala/blob/master/tests/custom_cluster/test_rpc_timeout.py#L145-L150 In general, we should converge on using DebugAction for any type of fault injection instead of building on the old infrastructure. http://gerrit.cloudera.org:8080/#/c/12260/3/be/src/service/control-service.cc@184 PS3, Line 184: FAULT_INJECTION_RPC_DELAY(RPC_REMOTESHUTDOWN); > These tests are setting delays on server-side commands so as to test RPC ti Please see replies 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: Thu, 31 Jan 2019 19:00:51 +0000 Gerrit-HasComments: Yes
