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 7: (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@645 PS7, Line 645: max_tries > nit: document what this variable represents Done. 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? I picked up a duration and yes this might need some tuning. I think it's probably better to wait before retrying right away in case of failures. 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? Removed the redundant variable. 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 I think we still need the retry logic for fetch. And, if the fetch rpcs fail, we may not have the correct fetch results (even if we try to retry fetch)? And so the only way to ensure that we are able to run the 'set all' query and get its results properly is by retrying the 'execute' and 'fetch' rpcs together as a whole. http://gerrit.cloudera.org:8080/#/c/15378/7/shell/impala_client.py@689 PS7, Line 689: {1} > nit: 'type={1}' Done. 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? Done. 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 ho Done. 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 Good point. I made changes so that we dump 'Num remaining tries <>' only if there is a remaining try. In other cases we dump nothing. The output looks like following: ``` Caught exception HTTP code 502: Injected Fault, type=<type 'exceptions.Exception'> in OpenSession. Num remaining tries: 2 Caught exception HTTP code 502: Injected Fault, type=<type 'exceptions.Exception'> in OpenSession. Num remaining tries: 1 Caught exception HTTP code 502: Injected Fault, type=<type 'exceptions.Exception'> in OpenSession. ``` 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? All the tests share the instance variable self.custom_hs2_http_client and self.transport. So probably not a good idea to run the tests in parallel. -- 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 21:34:31 +0000 Gerrit-HasComments: Yes
