Sahil Takiar has posted comments on this change. ( http://gerrit.cloudera.org:8080/15378 )
Change subject: IMPALA-9466: impala-shell client retry for hs2-http protocol ...................................................................... Patch Set 8: (8 comments) http://gerrit.cloudera.org:8080/#/c/15378/7/shell/impala_client.py File shell/impala_client.py: http://gerrit.cloudera.org:8080/#/c/15378/7/shell/impala_client.py@649 PS7, Line 649: # Wait duration (seconds) between retries. > I picked up a duration and yes this might need some tuning. I think it's pr yeah, this might not matter much, but unless there is a reason to wait, not sure it is necessary. i feel like if the underlying issue is connection resets due to dynamic reload by NGINX, then there isn't really a need to wait. a exponential backoff policy for the sleep duration would be nice as well, but again not sure if it makes much a difference - e.g. first and second try don't wait at all, third try can wait 2 seconds http://gerrit.cloudera.org:8080/#/c/15378/8/shell/impala_client.py File shell/impala_client.py: http://gerrit.cloudera.org:8080/#/c/15378/8/shell/impala_client.py@660 PS8, Line 660: return self.imp_service.OpenSession(open_session_req) nit: should be 2 spaces instead of 4? - same with the other methods http://gerrit.cloudera.org:8080/#/c/15378/8/shell/impala_client.py@661 PS8, Line 661: rpc = OpenSession is it necessary to define this intermediate variable, can OpenSession just be passed directly to '_do_hs2_rpc' - same with all the other methods http://gerrit.cloudera.org:8080/#/c/15378/8/shell/impala_client.py@678 PS8, Line 678: if not raise_error: : retry_msg = 'Num remaining tries: {0}'.format(self.max_tries - num_tries) : else: : retry_msg = '' if I'm reading this correctly, if retries are enabled. the first two tries will have: Num remaining tries: 2 Num remaining tries: 1 And the last try won't have anything. Would it be clearer if the last try had: Num remaining tries: 0 http://gerrit.cloudera.org:8080/#/c/15378/8/shell/impala_client.py@692 PS8, Line 692: except Exception, e: might be a nit, but what about QueryCancelledByShellException and MissingThriftMethodException, they are retried here, but not in do_hs2_rpc http://gerrit.cloudera.org:8080/#/c/15378/8/shell/impala_client.py@947 PS8, Line 947: Retries, if enabled, are attempted : for all exceptions other than QueryCancelledByShellException : and MissingThriftMethodException why are we retrying all TApplicationException (except for UNKNOWN_METHOD)? http://gerrit.cloudera.org:8080/#/c/15378/8/shell/impala_client.py@958 PS8, Line 958: retry_msg = 'Num remaining tries: {0}'.format(max_tries - num_tries) : else: : retry_msg = '' same comment as above http://gerrit.cloudera.org:8080/#/c/15378/8/shell/impala_client.py@974 PS8, Line 974: rpc.__name__ should this be included in the retry logic for the open session method? -- To view, visit http://gerrit.cloudera.org:8080/15378 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0da9e9e8d34a340eaf763397cc095ff6260d65d5 Gerrit-Change-Number: 15378 Gerrit-PatchSet: 8 Gerrit-Owner: Abhishek Rawat <[email protected]> Gerrit-Reviewer: Abhishek Rawat <[email protected]> Gerrit-Reviewer: David Knupp <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Sahil Takiar <[email protected]> Gerrit-Comment-Date: Wed, 18 Mar 2020 18:18:17 +0000 Gerrit-HasComments: Yes
