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 11: (6 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 ratio will be 0.3333333333333333, so the method will return 0.3. when num_tries = 2 it will return 2, and when num_tries = 3 it will return 2. not sure if that is your intention. i think a better retry policy would be: * first retry: don't sleep at all * second retry: sleep 1 second * third retry: sleep 2 seconds furthermore, I think this method should be robust enough so that it still returns a reasonable retry policy if the value of max_retries is changed. 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? http://gerrit.cloudera.org:8080/#/c/15378/11/shell/impala_client.py@701 PS11, Line 701: except Exception, e: wont this retry TApplicationException still? 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 lambda because the error message include the rpc.__name__ http://gerrit.cloudera.org:8080/#/c/15378/8/tests/custom_cluster/test_hs2_fault_injection.py File tests/custom_cluster/test_hs2_fault_injection.py: http://gerrit.cloudera.org:8080/#/c/15378/8/tests/custom_cluster/test_hs2_fault_injection.py@128 PS8, Line 128: OpenSession and CloseImpalaOperation rpcs fail. > I think the point of having a frequency is so that we have some faults and oh yeah duh 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 sense to add a dedicated method for it and just pass directly pass in the strings that should be matched to the output -- 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 <ara...@cloudera.com> Gerrit-Reviewer: Abhishek Rawat <ara...@cloudera.com> Gerrit-Reviewer: David Knupp <dkn...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Sahil Takiar <stak...@cloudera.com> Gerrit-Comment-Date: Mon, 23 Mar 2020 17:43:19 +0000 Gerrit-HasComments: Yes
