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 11: (7 comments) http://gerrit.cloudera.org:8080/#/c/15378/11/shell/impala_client.py File shell/impala_client.py: http://gerrit.cloudera.org:8080/#/c/15378/11/shell/impala_client.py@656 PS11, Line 656: if self.max_tries == 1: : return 0 : ratio = float(num_tries) / self.max_tries : if ratio < 0.3: : return 0.1 : elif ratio < 0.6: : return 0.3 : return 2 > if i'm reading this correctly, the first retry will have num_tries = 1, so The current logic basically has following (We have 3 tries total including the first one): 1s try if fail <sleep 0.3 s> 2nd try if fail <sleep 2 s> 3rd try if fail return error It's probably not that robust if someone uses 10 tries. But, we do need to be able to cap off the sleep time to a reasonable duration. I will think about a more robust function. http://gerrit.cloudera.org:8080/#/c/15378/11/shell/impala_client.py@697 PS11, Line 697: self.close_query(set_all_handle) > isn't this already retried? Also, this is redundant, since we already close the query in 'finally' block. I will remove this statement. http://gerrit.cloudera.org:8080/#/c/15378/11/shell/impala_client.py@701 PS11, Line 701: except Exception, e: > wont this retry TApplicationException still? It does, but we seem to be overusing RPCException for a variety of exceptions. If we get a TApplicationException, or a HTTP error code, or even if the impala server returns an error response, we throw RPCException. We could raise a different type of exception from _do_hs2_rpc, if we get a TApplicationException? But, that will also require changing handling the new type of exception in the impala_shell.py. http://gerrit.cloudera.org:8080/#/c/15378/11/shell/impala_client.py@935 PS11, Line 935: rpc > you might want to document that this should be a python function and not a Done http://gerrit.cloudera.org:8080/#/c/15378/10/tests/custom_cluster/test_hs2_fault_injection.py File tests/custom_cluster/test_hs2_fault_injection.py: http://gerrit.cloudera.org:8080/#/c/15378/10/tests/custom_cluster/test_hs2_fault_injection.py@136 PS10, Line 136: > flake8: E501 line too long (91 > 90 characters) Done http://gerrit.cloudera.org:8080/#/c/15378/10/tests/custom_cluster/test_hs2_fault_injection.py@237 PS10, Line 237: > flake8: E501 line too long (91 > 90 characters) Done http://gerrit.cloudera.org:8080/#/c/15378/11/tests/custom_cluster/test_hs2_fault_injection.py File tests/custom_cluster/test_hs2_fault_injection.py: http://gerrit.cloudera.org:8080/#/c/15378/11/tests/custom_cluster/test_hs2_fault_injection.py@302 PS11, Line 302: output = capsys.readouterr()[0].splitlines() : assert output[0] == ("Caught exception HTTP code 502: Injected Fault, " : "type=<type 'exceptions.Exception'> in GetLog. Num remaining tries: 2") > since this pattern is duplicated in several places, i think it would make s Done -- 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: 11 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: Mon, 23 Mar 2020 18:16:59 +0000 Gerrit-HasComments: Yes
