Abhishek Rawat 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: (9 comments) Addressed the comments so far... 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. > yeah, this might not matter much, but unless there is a reason to wait, not I think exponential backoff policy makes sense. 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 Done 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 Yeah we can pass the rpc functions directly. Done. 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 Done. 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 MissingTh Made the behavior consistent with 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)? I think for idempotent rpcs we could retry all exceptions. If we know retrying certain exceptions (such as MissingThriftMethodException) won't help, then we could skip them. http://gerrit.cloudera.org:8080/#/c/15378/8/shell/impala_client.py@954 PS8, Line 954: max_tries = self.max_tries > Nit: echoing a comment that Sahil made earlier, seems like this could be re I think in this case we could have rpcs with retry_on_error=True/False and so max_tries would depend on retry_on_error. The other alternative would be to explicitly pass 'max_tries' as param to '_do_hs2_rpc' like you had suggested earlier. Let me know, if you guys have a preference. 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 Done 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? Yeah, I think we should. The issue is that there is no easy way to determine if execute rpc failed or fetch rpc. We could use a generic message. -- 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 23:00:12 +0000 Gerrit-HasComments: Yes
