[Cosmos] add doc on client retry configs (#30171)

* new doc and small changes to other files

* Update _timeout_failover_retry_policy.py

* Update _timeout_failover_retry_policy.py

* fixing wrong changelog entry

* Update CHANGELOG.md

* Update ErrorCodesAndRetries.md

* Update CHANGELOG.md

* Update sdk/cosmos/azure-cosmos/docs/ErrorCodesAndRetries.md

Co-authored-by: Matias Quaranta <ealsur@users.noreply.github.com>

* Update sdk/cosmos/azure-cosmos/docs/ErrorCodesAndRetries.md

Co-authored-by: Matias Quaranta <ealsur@users.noreply.github.com>

* additional updates to doc, null check for request on retry policy

* small changes, removing /1000 for consistency as well

* updated docs with defaults, put in extra logic for request_timeout

* Update TimeoutAndRetriesConfig.md

---------

Co-authored-by: Matias Quaranta <ealsur@users.noreply.github.com>

Author: simorenoh

sdk/cosmos/azure-cosmos/CHANGELOG.md

@@ -7,8 +7,10 @@
 #### Breaking Changes
 
 #### Bugs Fixed
+* Fixed bug with non english locales causing an error with the RFC 1123 Date Format. See [PR 30125](https://github.com/Azure/azure-sdk-for-python/pull/30125).
 
 #### Other Changes
+* Refactoring of our client `connection_timeout` and `request_timeout` configurations. See [PR 30171](https://github.com/Azure/azure-sdk-for-python/pull/30171).
 
 ### 4.4.0b1 (2023-04-11)
 
@@ -21,7 +23,6 @@
 * Fixed bug in method `create_container_if_not_exists()` of async database client for unexpected kwargs being passed into `read()` method used internally. See [PR 29136](https://github.com/Azure/azure-sdk-for-python/pull/29136).
 * Fixed bug with method `query_items()` of our async container class, where partition key and cross partition headers would both be set when using partition keys. See [PR 29366](https://github.com/Azure/azure-sdk-for-python/pull/29366/).
 * Fixed bug with client not properly surfacing errors for invalid credentials and identities with insufficient permissions. Users running into 'NoneType has no attribute ConsistencyPolicy' errors when initializing their clients will now see proper authentication exceptions. See [PR 29256](https://github.com/Azure/azure-sdk-for-python/pull/29256).
-* Fixed bug with non english locales causing an error with the RFC 1123 Date Format. See [PR 30125](https://github.com/Azure/azure-sdk-for-python/pull/30125).
 
 #### Other Changes
 * Removed use of `six` package within the SDK.

sdk/cosmos/azure-cosmos/azure/cosmos/_cosmos_client_connection.py

@@ -160,7 +160,7 @@ def __init__(
                 retry_connect=self.connection_policy.ConnectionRetryConfiguration.connect,
                 retry_read=self.connection_policy.ConnectionRetryConfiguration.read,
                 retry_status=self.connection_policy.ConnectionRetryConfiguration.status,
-                retry_backoff_max=self.connection_policy.ConnectionRetryConfiguration.BACKOFF_MAX,
+                retry_backoff_max=self.connection_policy.ConnectionRetryConfiguration.backoff_max,
                 retry_on_status_codes=list(self.connection_policy.ConnectionRetryConfiguration.status_forcelist),
                 retry_backoff_factor=self.connection_policy.ConnectionRetryConfiguration.backoff_factor
             )

sdk/cosmos/azure-cosmos/azure/cosmos/_synchronized_request.py

@@ -84,7 +84,7 @@ def _Request(global_endpoint_manager, request_params, connection_policy, pipelin
     # pylint: disable=protected-access
 
     connection_timeout = connection_policy.RequestTimeout
-    connection_timeout = kwargs.pop("connection_timeout", connection_timeout / 1000.0)
+    connection_timeout = kwargs.pop("connection_timeout", connection_timeout)
 
     # Every request tries to perform a refresh
     client_timeout = kwargs.get('timeout')

sdk/cosmos/azure-cosmos/azure/cosmos/_timeout_failover_retry_policy.py

@@ -23,7 +23,6 @@
 Cosmos database service.
 """
 from . import http_constants
-from ..cosmos.documents import _OperationType
 
 
 class _TimeoutFailoverRetryPolicy(object):
@@ -38,19 +37,13 @@ def __init__(self, connection_policy, global_endpoint_manager, *args):
         self.failover_retry_count = 0
         self.connection_policy = connection_policy
         self.request = args[0] if args else None
-
-        if self.request:
-            self.request.clear_route_to_location()
-
-            # Resolve the endpoint for the request and pin the resolution to the resolved endpoint
-            # This enables marking the endpoint unavailability on endpoint failover/unreachability
-            self.location_endpoint = self.global_endpoint_manager.resolve_service_endpoint(self.request)
-            self.request.route_to_location(self.location_endpoint)
+        self.location_endpoint = self.global_endpoint_manager.resolve_service_endpoint(self.request)
 
     def needsRetry(self):
         if self.args:
             if (self.args[3].method == "GET") \
-                    or (http_constants.HttpHeaders.IsQueryPlanRequest in self.args[3].headers):
+                    or http_constants.HttpHeaders.IsQueryPlanRequest in self.args[3].headers\
+                    or http_constants.HttpHeaders.IsQuery in self.args[3].headers:
                 return True
         return False
 
@@ -75,27 +68,16 @@ def ShouldRetry(self, _exception):
 
         self.failover_retry_count += 1
 
-        if self.location_endpoint:
-            if _OperationType.IsReadOnlyOperation(self.request.operation_type):
-                # Mark current read endpoint as unavailable
-                self.global_endpoint_manager.mark_endpoint_unavailable_for_read(self.location_endpoint)
-            else:
-                self.global_endpoint_manager.mark_endpoint_unavailable_for_write(self.location_endpoint)
-
-        # set the refresh_needed flag to ensure that endpoint list is
-        # refreshed with new writable and readable locations
-        self.global_endpoint_manager.refresh_needed = True
-
-        # clear previous location-based routing directive
-        self.request.clear_route_to_location()
+        if self.request:
+            # clear previous location-based routing directive
+            self.request.clear_route_to_location()
 
-        # set location-based routing directive based on retry count
-        # simulating single master writes by ensuring usePreferredLocations
-        # is set to false
-        self.request.route_to_location_with_preferred_location_flag(self.failover_retry_count, False)
+            # set location-based routing directive based on retry count
+            # ensuring usePreferredLocations is set to True for retry
+            self.request.route_to_location_with_preferred_location_flag(self.failover_retry_count, True)
 
-        # Resolve the endpoint for the request and pin the resolution to the resolved endpoint
-        # This enables marking the endpoint unavailability on endpoint failover/unreachability
-        self.location_endpoint = self.global_endpoint_manager.resolve_service_endpoint(self.request)
-        self.request.route_to_location(self.location_endpoint)
+            # Resolve the endpoint for the request and pin the resolution to the resolved endpoint
+            # This enables marking the endpoint unavailability on endpoint failover/unreachability
+            self.location_endpoint = self.global_endpoint_manager.resolve_service_endpoint(self.request)
+            self.request.route_to_location(self.location_endpoint)
         return True

sdk/cosmos/azure-cosmos/azure/cosmos/aio/_asynchronous_request.py

@@ -53,7 +53,7 @@ async def _Request(global_endpoint_manager, request_params, connection_policy, p
     # pylint: disable=protected-access
 
     connection_timeout = connection_policy.RequestTimeout
-    connection_timeout = kwargs.pop("connection_timeout", connection_timeout / 1000.0)
+    connection_timeout = kwargs.pop("connection_timeout", connection_timeout)
 
     # Every request tries to perform a refresh
     client_timeout = kwargs.get('timeout')

sdk/cosmos/azure-cosmos/azure/cosmos/aio/_cosmos_client.py

@@ -45,31 +45,32 @@ def _build_connection_policy(kwargs: Dict[str, Any]) -> ConnectionPolicy:
     policy = kwargs.pop('connection_policy', None) or ConnectionPolicy()
 
     # Connection config
-    policy.RequestTimeout = kwargs.pop('request_timeout', None) or \
-                            kwargs.pop('connection_timeout', None) or \
-                            policy.RequestTimeout
-    policy.ConnectionMode = kwargs.pop('connection_mode', None) or policy.ConnectionMode
-    policy.ProxyConfiguration = kwargs.pop('proxy_config', None) or policy.ProxyConfiguration
-    policy.EnableEndpointDiscovery = kwargs.pop('enable_endpoint_discovery', None) or policy.EnableEndpointDiscovery
-    policy.PreferredLocations = kwargs.pop('preferred_locations', None) or policy.PreferredLocations
-    policy.UseMultipleWriteLocations = kwargs.pop('multiple_write_locations', None) or \
-                                       policy.UseMultipleWriteLocations
+    # `request_timeout` is supported as a legacy parameter later replaced by `connection_timeout`
+    if 'request_timeout' in kwargs:
+        policy.RequestTimeout = kwargs.pop('request_timeout') / 1000.0
+    else:
+        policy.RequestTimeout = kwargs.pop('connection_timeout', policy.RequestTimeout)
+    policy.ConnectionMode = kwargs.pop('connection_mode', policy.ConnectionMode)
+    policy.ProxyConfiguration = kwargs.pop('proxy_config', policy.ProxyConfiguration)
+    policy.EnableEndpointDiscovery = kwargs.pop('enable_endpoint_discovery', policy.EnableEndpointDiscovery)
+    policy.PreferredLocations = kwargs.pop('preferred_locations', policy.PreferredLocations)
+    policy.UseMultipleWriteLocations = kwargs.pop('multiple_write_locations', policy.UseMultipleWriteLocations)
 
     # SSL config
     verify = kwargs.pop('connection_verify', None)
     policy.DisableSSLVerification = not bool(verify if verify is not None else True)
     ssl = kwargs.pop('ssl_config', None) or policy.SSLConfiguration
     if ssl:
-        ssl.SSLCertFile = kwargs.pop('connection_cert', None) or ssl.SSLCertFile
+        ssl.SSLCertFile = kwargs.pop('connection_cert', ssl.SSLCertFile)
         ssl.SSLCaCerts = verify or ssl.SSLCaCerts
         policy.SSLConfiguration = ssl
 
     # Retry config
     retry_options = policy.RetryOptions
     total_retries = kwargs.pop('retry_total', None)
     retry_options._max_retry_attempt_count = total_retries or retry_options._max_retry_attempt_count
-    retry_options._fixed_retry_interval_in_milliseconds = kwargs.pop('retry_fixed_interval', None) or \
-        retry_options._fixed_retry_interval_in_milliseconds
+    retry_options._fixed_retry_interval_in_milliseconds = \
+        kwargs.pop('retry_fixed_interval', retry_options._fixed_retry_interval_in_milliseconds)
     max_backoff = kwargs.pop('retry_backoff_max', None)
     retry_options._max_wait_time_in_seconds = max_backoff or retry_options._max_wait_time_in_seconds
     policy.RetryOptions = retry_options
@@ -100,7 +101,7 @@ class CosmosClient(object):  # pylint: disable=client-accepts-api-version-keywor
     :keyword str consistency_level: Consistency level to use for the session. Default value is None (account-level).
         More on consistency levels and possible values: https://aka.ms/cosmos-consistency-levels
     :keyword int timeout: An absolute timeout in seconds, for the combined HTTP request and response processing.
-    :keyword int request_timeout: The HTTP request timeout in milliseconds.
+    :keyword int connection_timeout: The HTTP request timeout in seconds.
     :keyword str connection_mode: The connection mode for the client - currently only supports 'Gateway'.
     :keyword proxy_config: Connection proxy configuration.
     :paramtype proxy_config: ~azure.cosmos.ProxyConfiguration

sdk/cosmos/azure-cosmos/azure/cosmos/cosmos_client.py

@@ -74,26 +74,26 @@ def _build_auth(credential):
 def _build_connection_policy(kwargs):
     # type: (Dict[str, Any]) -> ConnectionPolicy
     # pylint: disable=protected-access
-    policy = kwargs.pop('connection_policy', None) or ConnectionPolicy()
+    policy = kwargs.pop('connection_policy', ConnectionPolicy())
 
     # Connection config
-    policy.RequestTimeout = kwargs.pop('request_timeout', None) or \
-        kwargs.pop('connection_timeout', None) or \
-        policy.RequestTimeout
-    policy.ConnectionMode = kwargs.pop('connection_mode', None) or policy.ConnectionMode
-    policy.ProxyConfiguration = kwargs.pop('proxy_config', None) or policy.ProxyConfiguration
-    policy.EnableEndpointDiscovery = kwargs.pop('enable_endpoint_discovery') \
-        if 'enable_endpoint_discovery' in kwargs.keys() else policy.EnableEndpointDiscovery
-    policy.PreferredLocations = kwargs.pop('preferred_locations', None) or policy.PreferredLocations
-    policy.UseMultipleWriteLocations = kwargs.pop('multiple_write_locations', None) or \
-        policy.UseMultipleWriteLocations
+    # `request_timeout` is supported as a legacy parameter later replaced by `connection_timeout`
+    if 'request_timeout' in kwargs:
+        policy.RequestTimeout = kwargs.pop('request_timeout') / 1000.0
+    else:
+        policy.RequestTimeout = kwargs.pop('connection_timeout', policy.RequestTimeout)
+    policy.ConnectionMode = kwargs.pop('connection_mode', policy.ConnectionMode)
+    policy.ProxyConfiguration = kwargs.pop('proxy_config', policy.ProxyConfiguration)
+    policy.EnableEndpointDiscovery = kwargs.pop('enable_endpoint_discovery', policy.EnableEndpointDiscovery)
+    policy.PreferredLocations = kwargs.pop('preferred_locations', policy.PreferredLocations)
+    policy.UseMultipleWriteLocations = kwargs.pop('multiple_write_locations', policy.UseMultipleWriteLocations)
 
     # SSL config
     verify = kwargs.pop('connection_verify', None)
     policy.DisableSSLVerification = not bool(verify if verify is not None else True)
-    ssl = kwargs.pop('ssl_config', None) or policy.SSLConfiguration
+    ssl = kwargs.pop('ssl_config', policy.SSLConfiguration)
     if ssl:
-        ssl.SSLCertFile = kwargs.pop('connection_cert', None) or ssl.SSLCertFile
+        ssl.SSLCertFile = kwargs.pop('connection_cert', ssl.SSLCertFile)
         ssl.SSLCaCerts = verify or ssl.SSLCaCerts
         policy.SSLConfiguration = ssl
 
@@ -141,7 +141,7 @@ class CosmosClient(object):  # pylint: disable=client-accepts-api-version-keywor
     :param str consistency_level: Consistency level to use for the session. The default value is None (Account level).
         More on consistency levels and possible values: https://aka.ms/cosmos-consistency-levels
     :keyword int timeout: An absolute timeout in seconds, for the combined HTTP request and response processing.
-    :keyword int request_timeout: The HTTP request timeout in milliseconds.
+    :keyword int connection_timeout: The HTTP request timeout in seconds.
     :keyword str connection_mode: The connection mode for the client - currently only supports 'Gateway'.
     :keyword proxy_config: Connection proxy configuration.
     :paramtype proxy_config: ~azure.cosmos.ProxyConfiguration

sdk/cosmos/azure-cosmos/azure/cosmos/documents.py

@@ -358,7 +358,7 @@ class ConnectionPolicy(object):  # pylint: disable=too-many-instance-attributes
         int or azure.cosmos.ConnectionRetryPolicy or urllib3.util.retry
     """
 
-    __defaultRequestTimeout = 60000  # milliseconds
+    __defaultRequestTimeout = 60  # seconds
 
     def __init__(self):
         self.RequestTimeout = self.__defaultRequestTimeout