Sahil Takiar has posted comments on this change. ( http://gerrit.cloudera.org:8080/14246 )
Change subject: IMPALA-8634: Catalog client should retry RPCs ...................................................................... Patch Set 1: (3 comments) http://gerrit.cloudera.org:8080/#/c/14246/1/be/src/runtime/exec-env.cc File be/src/runtime/exec-env.cc: http://gerrit.cloudera.org:8080/#/c/14246/1/be/src/runtime/exec-env.cc@142 PS1, Line 142: DEFINE_int32(catalog_client_connection_num_retries, 3, "The number of times connections " > Seems reasonable to me. Done, interestingly 10 retries and a 3 second interval is what the statestore client is already using. http://gerrit.cloudera.org:8080/#/c/14246/1/be/src/runtime/exec-env.cc@144 PS1, Line 144: 0 > We may want to consider setting this timeout to non-zero at some point. Whe Sure, this patch or a separate one? Not sure what an appropriate default for this would be. http://gerrit.cloudera.org:8080/#/c/14246/1/be/src/runtime/exec-env.cc@180 PS1, Line 180: 1, 0 > May help to name these constants as variables. I added some code comments which I think makes it clearer than introducing variables like `catalog_client_connection_num_retries` and `catalog_client_rpc_retry_interval_ms`. It still tries to recreate new connections, but the retry is driven by the DoRpcWithRetry() loop rather than the loop in ThriftClientImpl::OpenWithRetry. The loop in DoRpcWithRetry() first tries to create a new ClientConnection, that will trigger a call to ClientCacheHelper::GetClient, which will either try to create a new connection or used a cached one. If it tries to get a new connection and the catalogd is down, the connection will fail, but the next iteration of the loop in DoRpcWithRetry() will retry to establish the connection. If GetClient finds a cached connection, the RPC will fail, and again the loop will trigger a retry. -- To view, visit http://gerrit.cloudera.org:8080/14246 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7f33ad2b36d301fb64e70a939e71decab0ca993c Gerrit-Change-Number: 14246 Gerrit-PatchSet: 1 Gerrit-Owner: Sahil Takiar <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Sahil Takiar <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Wed, 18 Sep 2019 19:53:32 +0000 Gerrit-HasComments: Yes
