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 8: (10 comments) 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@947 PS8, Line 947: Retries, if enabled, are attempted : for all exceptions other than QueryCancelledByShellException : and MissingThriftMethodException > I think for idempotent rpcs we could retry all exceptions. If we know retry My understanding of TApplicationExceptions is that they are thrown by the Thrift server. So it wouldn't be thrown for connection issues. I think connection issues are encapsulated by TTransportException. I'm cautious of retrying all exceptions, even for idempotent methods. For one, from a user perspective, it makes it sound like the issue is possibly intermittent and retries are expected to fix the issue, but that isn't that case for all exceptions. 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 > All the tests share the instance variable self.custom_hs2_http_client and s you could probably use a separate connection for each test, but I guess either way is fine since these tests shouldn't run for that long. 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@30 PS8, Line 30: """Class for injecting faults in the THttpClient.""" should include an explanation of how faults are injected (e.g. they are injected by overriding the flush method) as well as how to inject faults http://gerrit.cloudera.org:8080/#/c/15378/8/tests/custom_cluster/test_hs2_fault_injection.py@40 PS8, Line 40: fault nit: enable_fault would sound clearer http://gerrit.cloudera.org:8080/#/c/15378/8/tests/custom_cluster/test_hs2_fault_injection.py@81 PS8, Line 81: """Fault injecting ImpalaHS2Client class using http as transport""" should mention the relation between this class and FaultInjectingHttpClient - specifically that this client injects faults by using the FaultInjectingHttpClient as the transport http://gerrit.cloudera.org:8080/#/c/15378/8/tests/custom_cluster/test_hs2_fault_injection.py@85 PS8, Line 85: """Creates a transport with HTTP as the base.""" nit: should be a code comment, so should start with '#' instead of """ http://gerrit.cloudera.org:8080/#/c/15378/8/tests/custom_cluster/test_hs2_fault_injection.py@101 PS8, Line 101: CustomClusterTestSuite why does this need to be a custom cluster test? you are just injecting faults into the client, so this can run against a normal impala cluster, right? http://gerrit.cloudera.org:8080/#/c/15378/8/tests/custom_cluster/test_hs2_fault_injection.py@106 PS8, Line 106: kerberos_host_fqdn = None why is this needed, just pass None as a parameter? http://gerrit.cloudera.org:8080/#/c/15378/8/tests/custom_cluster/test_hs2_fault_injection.py@128 PS8, Line 128: self.transport.fault(502, "Injected Fault", 0.20) whats the point in adding a frequency to these tests? why not always inject the fault? http://gerrit.cloudera.org:8080/#/c/15378/8/tests/custom_cluster/test_hs2_fault_injection.py@129 PS8, Line 129: self.connect() is there a way to actually verify that operations were retried? -- 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: Thu, 19 Mar 2020 16:59:34 +0000 Gerrit-HasComments: Yes
