Revert all changes since main

susodapop · susodapop · commit fa1fd50c4388 · 2022-08-04T10:20:32.000-05:00
Signed-off-by: Jesse Whitehouse &lt;jesse@whitehouse.dev&gt;
diff --git a/src/databricks/sql/thrift_backend.py b/src/databricks/sql/thrift_backend.py
@@ -5,7 +5,6 @@
 import threading
 from uuid import uuid4
 from ssl import CERT_NONE, CERT_OPTIONAL, CERT_REQUIRED, create_default_context
-import socket, types
 
 import pyarrow
 import thrift.transport.THttpClient
@@ -40,7 +39,6 @@
     "_retry_delay_max": (float, 60, 5, 3600),
     "_retry_stop_after_attempts_count": (int, 30, 1, 60),
     "_retry_stop_after_attempts_duration": (float, 900, 1, 86400),
-    "_retry_delay_default": (float, 5, 1, 60),
 }
 
 
@@ -81,8 +79,6 @@ def __init__(
         #   next calculated pre-retry delay would go past
         #   _retry_stop_after_attempts_duration, stop now.)
         #
-        # _retry_delay_default                   (default: 5)
-        #   used when Retry-After is not specified by the server
         # _retry_stop_after_attempts_count
         #  The maximum number of times we should retry retryable requests (defaults to 24)
         # _socket_timeout
@@ -247,7 +243,7 @@ def _handle_request_error(self, error_info, attempt, elapsed):
     # FUTURE: Consider moving to https://github.com/litl/backoff or
     # https://github.com/jd/tenacity for retry logic.
     def make_request(self, method, request):
-        """Execute given request, attempting retries when TCP connection fils or when receiving HTTP 429/503.
+        """Execute given request, attempting retries when receiving HTTP 429/503.
 
         For delay between attempts, honor the given Retry-After header, but with bounds.
         Use lower bound of expontial-backoff based on _retry_delay_min,
@@ -265,12 +261,8 @@ def get_elapsed():
             return time.time() - t0
 
         def extract_retry_delay(attempt):
-            """
-            Encapsulate retry checks based on HTTP headers. Returns None || delay-in-secs
-
-            Retry IFF 429/503 code + Retry-After header set
-            """
-
+            # encapsulate retry checks, returns None || delay-in-secs
+            # Retry IFF 429/503 code + Retry-After header set
             http_code = getattr(self._transport, "code", None)
             retry_after = getattr(self._transport, "headers", {}).get("Retry-After")
             if http_code in [429, 503] and retry_after:
@@ -287,63 +279,24 @@ def attempt_request(attempt):
             # - non-None method_return -> success, return and be done
             # - non-None retry_delay -> sleep delay before retry
             # - error, error_message always set when available
-
-            error = None
-
-            # If retry_delay is None the request is treated as non-retryable
-            retry_delay = None
             try:
                 logger.debug("Sending request: {}".format(request))
                 response = method(request)
                 logger.debug("Received response: {}".format(response))
                 return response
-            except socket.timeout as err:
-                # We only retry for socket.timeout if the operation that timed out was a connection request
-                # Otherwise idempotency is not guaranteed because something may have been transmitted to the server
-                
-                def _dig_through_traceback(tb: types.TracebackType, mod, meth):
-                    """Recursively search the traceback stack to see if mod.meth raised the exception
-                    """
-                    _mod, _meth = mod, meth
-                    tb_meth = tb.tb_frame.f_code.co_name
-                    tb_mod = tb.tb_frame.f_code.co_filename.split("/")[-1].replace(".py", "")
-
-                    if tb_meth == _meth and _mod == tb_mod:
-                        return True
-                    elif tb.tb_next is None:
-                        return False
-
-                    return _dig_through_traceback(tb.tb_next, mod, meth)
-
-                tb = err.__traceback__
-                failed_during_socket_connect = _dig_through_traceback(tb, "socket", "create_connection")
-                failed_during_http_open = _dig_through_traceback(tb, "client", "connect") 
-                
-                if failed_during_socket_connect and failed_during_http_open:
-                    retry_delay = self._retry_delay_default
-
-                error_message = str(err)
-                error = err
-            except OSError as err:
-                # OSError 110 means EHOSTUNREACHABLE, which means the connection was timed out by the operating system
-                if "Errno 110" in str(err):
-                    retry_delay = self._retry_delay_default
-                error_message = str(err)
-                error = err
-            except Exception as err:
+            except Exception as error:
                 retry_delay = extract_retry_delay(attempt)
                 error_message = ThriftBackend._extract_error_message_from_headers(
                     getattr(self._transport, "headers", {})
                 )
-                error = err
-            return RequestErrorInfo(
-                error=error,
-                error_message=error_message,
-                retry_delay=retry_delay,
-                http_code=getattr(self._transport, "code", None),
-                method=method.__name__,
-                request=request,
-            )
+                return RequestErrorInfo(
+                    error=error,
+                    error_message=error_message,
+                    retry_delay=retry_delay,
+                    http_code=getattr(self._transport, "code", None),
+                    method=method.__name__,
+                    request=request,
+                )
 
         # The real work:
         # - for each available attempt:
diff --git a/tests/e2e/driver_tests.py b/tests/e2e/driver_tests.py
@@ -7,7 +7,7 @@
 import sys
 import threading
 import time
-from unittest import loader, skipIf, skipUnless, TestCase, mock
+from unittest import loader, skipIf, skipUnless, TestCase
 from uuid import uuid4
 
 import numpy as np
@@ -360,66 +360,6 @@ def execute_really_long_query():
             cursor.execute("SELECT * FROM range(3)")
             self.assertEqual(len(cursor.fetchall()), 3)
 
-    def test_retry_after_connection_timeout(self):
-        # We only retry a request that failed because of a socket timeout in this case
-        # when the timeout occurred while trying to connect with the thrift server.
-        # In this situation, we know that a command was not sent to the server because
-        # no connection was made.
-        
-        import socket
-        from unittest.mock import Mock
-        from databricks.sql.thrift_api.TCLIService import ttypes
-
-       # First let's check the non-retry behavior
-       # Given the client has successfully connected to the server already
-       # When a socket.timeout exception is raised
-       # Then the request is not retried
-        with self.assertRaises(OperationalError) as cm:
-            
-            # No mocks at this point. If calling self.cursor() succeeds
-            # that means there is an open / working connection to thrift server.
-            with self.cursor({}) as cursor:
-
-                # Next apply a patch to the transport which raises a socket.timeout
-                # whenever any data is sent over the wire
-                with mock.patch("http.client.HTTPConnection.send") as mock_send:
-                    mock_send.side_effect = socket.timeout
-                    cursor.execute("I AM A VERY DANGEROUS, NOT IDEMPOTENT QUERY!!!")
-            self.assertIn("non-retryable error", cm.exception.message_with_context())
-
-
-        # Second, let's check whether a request is retried if it fails during
-        # the connection attempt, instead of the send attempt.
-        
-        ATTEMPTS_TO_TRY = 2
-
-        # This is a normal query execution
-        # with self.cursor({}) as cursor:
-        #     thrift_backend = cursor.thrift_backend
-
-        
-        with self.assertRaises(OperationalError) as cm:
-            with mock.patch("socket.socket.connect") as mock_connect:
-                mock_connect.side_effect = OSError("[Errno 110]: Connection timed out")
-                with self.cursor() as cursor:
-                    # Connection will fail
-                    cursor.execute("SOME RANDOM STATEMENT")
-                    pass
-
-            self.assertIn("{0}/{0}".format(ATTEMPTS_TO_TRY), cm.exception.message_with_context())
-
-        with self.assertRaises(OperationalError) as cm:
-            with mock.patch("socket.socket.connect") as mock_connect:
-                mock_connect.side_effect = socket.timeout
-                with self.cursor() as cursor:
-                    # Connection will fail
-                    cursor.execute("SOME RANDOM STATEMENT")
-                    pass
-
-            self.assertIn("{0}/{0}".format(ATTEMPTS_TO_TRY), cm.exception.message_with_context())
-
-
-
     @skipIf(pysql_has_version('<', '2'), 'requires pysql v2')
     def test_can_execute_command_after_failure(self):
         with self.cursor({}) as cursor:
diff --git a/tests/unit/test_thrift_backend.py b/tests/unit/test_thrift_backend.py
@@ -19,7 +19,6 @@ def retry_policy_factory():
         "_retry_delay_max":                     (float, 60, None, None),
         "_retry_stop_after_attempts_count":     (int, 30, None, None),
         "_retry_stop_after_attempts_duration":  (float, 900, None, None),
-        "_retry_delay_default":                 (float, 5, 0, 10),
     }
 
 
@@ -985,63 +984,6 @@ def test_make_request_wont_retry_if_headers_not_present(self, t_transport_class)
 
         self.assertIn("This method fails", str(cm.exception.message_with_context()))
 
-    @patch("thrift.transport.THttpClient.THttpClient")
-    def test_will_retry_on_connection_timeout(self, t_transport_class):
-
-        import socket
-
-        mock_method = Mock()
-        mock_method.__name__ = "method name"
-        mock_method.side_effect = socket.timeout
-
-        thrift_backend = ThriftBackend(
-            "foobar",
-            443,
-            "path",
-            [],
-            _retry_stop_after_attempts_count=2,
-            _retry_delay_default=0.25
-        )
-
-        with self.assertRaises(OperationalError) as cm:
-            thrift_backend.make_request(mock_method, Mock())
-
-            self.assertIn("2/2", cm.exception.message_with_context())
-
-        mock_method = Mock()
-        mock_method.__name__ = "method name"
-        mock_method.side_effect = OSError("[Errno 110] Connection timed out")
-
-        with self.assertRaises(OperationalError) as cm:
-            thrift_backend.make_request(mock_method, Mock())
-
-            self.assertIn("2/2", cm.exception.message_with_context())
-
-    @patch("thrift.transport.THttpClient.THttpClient")
-    def test_will_not_retry_on_non_timeout_oserror(self, t_transport_class):
-
-        
-
-        mock_method = Mock()
-        mock_method.__name__ = "method name"
-        mock_method.side_effect = OSError("I am not a timeout error")
-
-        thrift_backend = ThriftBackend(
-            "foobar",
-            443,
-            "path",
-            [],
-            _retry_stop_after_attempts_count=2,
-            _retry_delay_default=0.25
-        )
-
-        with self.assertRaises(OperationalError) as cm:
-            thrift_backend.make_request(mock_method, Mock())
-
-        self.assertIn("I am not a timeout error", str(cm.exception.message_with_context()))
-        self.assertIn("1/2", cm.exception.message_with_context())
-
-
     @patch("thrift.transport.THttpClient.THttpClient")
     def test_make_request_wont_retry_if_error_code_not_429_or_503(self, t_transport_class):
         t_transport_instance = t_transport_class.return_value
