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 7: (9 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@645 PS7, Line 645: max_tries nit: document what this variable represents http://gerrit.cloudera.org:8080/#/c/15378/7/shell/impala_client.py@649 PS7, Line 649: self.retry_sleep_duration_s = 2 why 2 seconds? http://gerrit.cloudera.org:8080/#/c/15378/7/shell/impala_client.py@674 PS7, Line 674: max_tries = self.max_tries nit: why is this necessary? http://gerrit.cloudera.org:8080/#/c/15378/7/shell/impala_client.py@679 PS7, Line 679: execute_query would it make sense to add an option to execute_query that adds the option 'retry_on_error' and just pass it through to '_do_hs2_rpc'? that way we don't have to implement the retry logic twice. once here and again in _do_hs2_rpc http://gerrit.cloudera.org:8080/#/c/15378/7/shell/impala_client.py@689 PS7, Line 689: {1} nit: 'type={1}' http://gerrit.cloudera.org:8080/#/c/15378/7/shell/impala_client.py@693 PS7, Line 693: if set_all_handle is not None: : self.close_query(set_all_handle) should this be in a finally block? http://gerrit.cloudera.org:8080/#/c/15378/7/shell/impala_client.py@934 PS7, Line 934: """Executes the provided 'rpc' callable and tranlates any exceptions in the nit: document new option, would recommend including some docs explaining how self.max_tries and retry_on_error interact http://gerrit.cloudera.org:8080/#/c/15378/7/shell/impala_client.py@960 PS7, Line 960: Num remaining tries: {3} i think this is potentially confusing to this for all rpcs, especially for those that can't be retried. from a client perspective, it makes it sound like the rpc can be retried and the retries were exhaustive, when in reality the rpc was not retried at all http://gerrit.cloudera.org:8080/#/c/15378/7/tests/custom_cluster/test_hs2_fault_injection.py File tests/custom_cluster/test_hs2_fault_injection.py: http://gerrit.cloudera.org:8080/#/c/15378/7/tests/custom_cluster/test_hs2_fault_injection.py@123 PS7, Line 123: @pytest.mark.execute_serially why do these all need to be executed serially? -- 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: 7 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: Tue, 17 Mar 2020 18:12:52 +0000 Gerrit-HasComments: Yes
