Address review feedback: flat rest.client.* connection properties

- Rename the connection/retry/timeout config to flat rest.client.* keys (rest.client.request-timeout, rest.client.max-retries, rest.client.retry-backoff-factor), aligning naming with the Java HTTPClient while keeping seconds and a backoff-factor option.
- Use requests' DEFAULT_RETRIES and drop the None branch in the adapter constructor.
- Extract a helper to remove the repeated parse-and-validate logic across the three properties.
- Retry idempotent PUT and DELETE in addition to GET/HEAD/OPTIONS.

Author: rahulsmahadev

mkdocs/docs/configuration.md

@@ -352,25 +352,24 @@ catalog:
 
 The REST Catalog uses `requests` with no retries and no timeout by default, so transient
 5xx / network failures bubble up immediately and slow servers can hang the client indefinitely.
-Set a `connection:` block on the catalog to opt in to a per-request timeout and a retry policy.
+Set the `rest.client.*` catalog properties to opt in to a request timeout and a retry policy.
 
 ```yaml
 catalog:
   default:
     uri: http://rest-catalog/ws/
-    connection:
-      timeout: 60          # seconds, applied to every HTTP call
-      retries: 5           # number of retry attempts on transient failures
-      backoff-factor: 1.0  # exponential backoff between retries
+    rest.client.request-timeout: 60       # seconds, applied to the whole request
+    rest.client.max-retries: 5            # number of retry attempts on transient failures
+    rest.client.retry-backoff-factor: 1.0 # exponential backoff between retries
 ```
 
-| Key                          | Example  | Description                                                                                                        |
-| ---------------------------- | -------- | ------------------------------------------------------------------------------------------------------------------ |
-| connection.timeout           | 60       | Per-request timeout in seconds. Must be a positive number.                                                         |
-| connection.retries           | 5        | Number of retry attempts for transient failures. Must be non-negative.                                             |
-| connection.backoff-factor    | 1.0      | Backoff factor between retry attempts. Must be non-negative. See [`urllib3` Retry docs](https://urllib3.readthedocs.io/en/latest/reference/urllib3.util.html#urllib3.util.Retry) for the formula. |
+| Key                                     | Example  | Description                                                                                                        |
+| --------------------------------------- | -------- | ------------------------------------------------------------------------------------------------------------------ |
+| rest.client.request-timeout             | 60       | Timeout in seconds, applied as a single value to the whole request. Must be a positive number.                      |
+| rest.client.max-retries                 | 5        | Number of retry attempts for transient failures. Must be non-negative.                                             |
+| rest.client.retry-backoff-factor        | 1.0      | Backoff factor between retry attempts. Must be non-negative. See [`urllib3` Retry docs](https://urllib3.readthedocs.io/en/latest/reference/urllib3.util.html#urllib3.util.Retry) for the formula. |
 
-Retries are applied to idempotent methods only (`GET`, `HEAD`, `OPTIONS`) and to the
+Retries are applied to idempotent methods only (`GET`, `HEAD`, `OPTIONS`, `PUT`, `DELETE`) and to the
 transient HTTP status codes `429`, `500`, `502`, `503`, `504`. Other failures are not retried.
 
 #### Headers in REST Catalog

pyiceberg/catalog/rest/__init__.py

@@ -17,17 +17,18 @@
 from __future__ import annotations
 
 from collections import deque
-from collections.abc import Mapping
+from collections.abc import Callable, Mapping
 from enum import Enum
 from typing import (
     TYPE_CHECKING,
     Any,
+    TypeVar,
 )
 from urllib.parse import quote, unquote
 
 from pydantic import ConfigDict, Field, TypeAdapter, field_validator
 from requests import HTTPError, PreparedRequest, Response, Session
-from requests.adapters import HTTPAdapter
+from requests.adapters import DEFAULT_RETRIES, HTTPAdapter
 from tenacity import RetryCallState, retry, retry_if_exception_type, stop_after_attempt
 from typing_extensions import override
 from urllib3.util.retry import Retry
@@ -258,14 +259,13 @@ class ScanPlanningMode(Enum):
 SIGV4_SERVICE = "rest.signing-name"
 SIGV4_MAX_RETRIES = "rest.sigv4.max-retries"
 SIGV4_MAX_RETRIES_DEFAULT = 10
-CONNECTION = "connection"
-CONNECTION_TIMEOUT = "timeout"
-CONNECTION_RETRIES = "retries"
-CONNECTION_BACKOFF_FACTOR = "backoff-factor"
+REST_CLIENT_REQUEST_TIMEOUT = "rest.client.request-timeout"
+REST_CLIENT_MAX_RETRIES = "rest.client.max-retries"
+REST_CLIENT_RETRY_BACKOFF_FACTOR = "rest.client.retry-backoff-factor"
 # Hard-coded internally so users cannot misconfigure the retry policy
 # (e.g. setting raise_on_status=False would swallow 4xx errors silently).
 _CONNECTION_RETRY_STATUS_FORCELIST = (429, 500, 502, 503, 504)
-_CONNECTION_RETRY_ALLOWED_METHODS = frozenset({"GET", "HEAD", "OPTIONS"})
+_CONNECTION_RETRY_ALLOWED_METHODS = frozenset({"GET", "HEAD", "OPTIONS", "PUT", "DELETE"})
 EMPTY_BODY_SHA256: str = "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855"
 OAUTH2_SERVER_URI = "oauth2-server-uri"
 SNAPSHOT_LOADING_MODE = "snapshot-loading-mode"
@@ -403,6 +403,29 @@ class ListViewsResponse(IcebergBaseModel):
 _PLANNING_RESPONSE_ADAPTER = TypeAdapter(PlanningResponse)
 
 
+_T = TypeVar("_T", int, float)
+
+
+def _parse_connection_property(
+    properties: Properties,
+    property_name: str,
+    converter: Callable[[Any], _T],
+    type_description: str,
+    is_invalid: Callable[[_T], bool],
+    range_description: str,
+) -> _T | None:
+    raw_value = properties.get(property_name)
+    if raw_value is None:
+        return None
+    try:
+        value = converter(raw_value)
+    except (TypeError, ValueError) as e:
+        raise ValueError(f"`{property_name}` must be {type_description}, got: {raw_value!r}") from e
+    if is_invalid(value):
+        raise ValueError(f"`{property_name}` must be {range_description}, got: {value}")
+    return value
+
+
 class _RetryTimeoutHTTPAdapter(HTTPAdapter):
     """HTTPAdapter that applies a default per-request timeout.
 
@@ -411,12 +434,9 @@ class _RetryTimeoutHTTPAdapter(HTTPAdapter):
     The adapter applies `self._timeout` whenever a per-call timeout is not set.
     """
 
-    def __init__(self, timeout: float | None = None, max_retries: Retry | int | None = None) -> None:
+    def __init__(self, timeout: float | None = None, max_retries: Retry | int = DEFAULT_RETRIES) -> None:
         self._timeout = timeout
-        if max_retries is not None:
-            super().__init__(max_retries=max_retries)
-        else:
-            super().__init__()
+        super().__init__(max_retries=max_retries)
 
     def send(
         self,
@@ -433,51 +453,50 @@ def send(
 
 
 def _create_connection_adapter(properties: Properties) -> _RetryTimeoutHTTPAdapter | None:
-    """Build a connection adapter from the optional `connection.*` properties.
+    """Build a connection adapter from the optional `rest.client.*` properties.
 
-    Returns None when no `connection` block is supplied, leaving the default
+    Returns None when no connection properties are supplied, leaving the default
     Session behavior unchanged. Raises ValueError on invalid input.
     """
-    connection_config = properties.get(CONNECTION)
-    if not connection_config:
+    if not any(
+        property_name in properties
+        for property_name in (REST_CLIENT_REQUEST_TIMEOUT, REST_CLIENT_MAX_RETRIES, REST_CLIENT_RETRY_BACKOFF_FACTOR)
+    ):
         return None
-    if not isinstance(connection_config, dict):
-        raise ValueError(f"`{CONNECTION}` must be a mapping, got: {type(connection_config).__name__}")
 
-    timeout: float | None = None
-    if (raw_timeout := connection_config.get(CONNECTION_TIMEOUT)) is not None:
-        try:
-            timeout = float(raw_timeout)
-        except (TypeError, ValueError) as e:
-            raise ValueError(f"`{CONNECTION}.{CONNECTION_TIMEOUT}` must be a number, got: {raw_timeout!r}") from e
-        if timeout <= 0:
-            raise ValueError(f"`{CONNECTION}.{CONNECTION_TIMEOUT}` must be a positive number, got: {timeout}")
-
-    # `retries` and `backoff_factor` default to 0 (a no-op Retry) so the user can set only
-    # one or the other without forcing the rest of the policy to be specified explicitly.
-    retries = 0
-    if (raw_retries := connection_config.get(CONNECTION_RETRIES)) is not None:
-        try:
-            retries = int(raw_retries)
-        except (TypeError, ValueError) as e:
-            raise ValueError(f"`{CONNECTION}.{CONNECTION_RETRIES}` must be an integer, got: {raw_retries!r}") from e
-        if retries < 0:
-            raise ValueError(f"`{CONNECTION}.{CONNECTION_RETRIES}` must be non-negative, got: {retries}")
-
-    backoff_factor = 0.0
-    if (raw_backoff := connection_config.get(CONNECTION_BACKOFF_FACTOR)) is not None:
-        try:
-            backoff_factor = float(raw_backoff)
-        except (TypeError, ValueError) as e:
-            raise ValueError(f"`{CONNECTION}.{CONNECTION_BACKOFF_FACTOR}` must be a number, got: {raw_backoff!r}") from e
-        if backoff_factor < 0:
-            raise ValueError(f"`{CONNECTION}.{CONNECTION_BACKOFF_FACTOR}` must be non-negative, got: {backoff_factor}")
+    timeout = _parse_connection_property(
+        properties,
+        REST_CLIENT_REQUEST_TIMEOUT,
+        float,
+        "a number",
+        lambda value: value <= 0,
+        "a positive number",
+    )
+
+    retries = _parse_connection_property(
+        properties,
+        REST_CLIENT_MAX_RETRIES,
+        int,
+        "an integer",
+        lambda value: value < 0,
+        "non-negative",
+    )
+    backoff_factor = _parse_connection_property(
+        properties,
+        REST_CLIENT_RETRY_BACKOFF_FACTOR,
+        float,
+        "a number",
+        lambda value: value < 0,
+        "non-negative",
+    )
 
     return _RetryTimeoutHTTPAdapter(
         timeout=timeout,
         max_retries=Retry(
-            total=retries,
-            backoff_factor=backoff_factor,
+            # `retries` and `backoff_factor` fall back to a no-op Retry when unset, so a user can
+            # configure only one without having to specify the rest of the policy.
+            total=retries if retries is not None else DEFAULT_RETRIES,
+            backoff_factor=backoff_factor if backoff_factor is not None else 0.0,
             status_forcelist=list(_CONNECTION_RETRY_STATUS_FORCELIST),
             allowed_methods=_CONNECTION_RETRY_ALLOWED_METHODS,
             # Return the final response on retry exhaustion (instead of raising MaxRetryError)

tests/catalog/test_rest.py

@@ -33,13 +33,12 @@
 import pyiceberg
 from pyiceberg.catalog import PropertiesUpdateSummary, load_catalog
 from pyiceberg.catalog.rest import (
-    CONNECTION,
-    CONNECTION_BACKOFF_FACTOR,
-    CONNECTION_RETRIES,
-    CONNECTION_TIMEOUT,
     DEFAULT_ENDPOINTS,
     EMPTY_BODY_SHA256,
     OAUTH2_SERVER_URI,
+    REST_CLIENT_MAX_RETRIES,
+    REST_CLIENT_REQUEST_TIMEOUT,
+    REST_CLIENT_RETRY_BACKOFF_FACTOR,
     SIGV4_MAX_RETRIES,
     SIGV4_MAX_RETRIES_DEFAULT,
     SNAPSHOT_LOADING_MODE,
@@ -2036,11 +2035,9 @@ def test_session_with_connection_timeout_and_retries(rest_mock: Mocker) -> None:
     catalog_properties = {
         "uri": TEST_URI,
         "token": TEST_TOKEN,
-        CONNECTION: {
-            CONNECTION_TIMEOUT: 60,
-            CONNECTION_RETRIES: 5,
-            CONNECTION_BACKOFF_FACTOR: 1.0,
-        },
+        REST_CLIENT_REQUEST_TIMEOUT: 60,
+        REST_CLIENT_MAX_RETRIES: 5,
+        REST_CLIENT_RETRY_BACKOFF_FACTOR: 1.0,
     }
     catalog = RestCatalog("rest", **catalog_properties)  # type: ignore
 
@@ -2054,14 +2051,14 @@ def test_session_with_connection_timeout_and_retries(rest_mock: Mocker) -> None:
     # Internal retry policy: transient codes and idempotent methods only.
     assert https_adapter.max_retries.status_forcelist == [429, 500, 502, 503, 504]
     allowed_methods = https_adapter.max_retries.allowed_methods or frozenset()
-    assert set(allowed_methods) == {"GET", "HEAD", "OPTIONS"}
+    assert set(allowed_methods) == {"GET", "HEAD", "OPTIONS", "PUT", "DELETE"}
 
 
 def test_session_with_connection_timeout_only(rest_mock: Mocker) -> None:
     catalog_properties = {
         "uri": TEST_URI,
         "token": TEST_TOKEN,
-        CONNECTION: {CONNECTION_TIMEOUT: "30"},
+        REST_CLIENT_REQUEST_TIMEOUT: "30",
     }
     catalog = RestCatalog("rest", **catalog_properties)  # type: ignore
     adapter = catalog._session.adapters["https://"]
@@ -2132,7 +2129,8 @@ def test_session_retries_on_transient_5xx_then_succeeds() -> None:
                 "uri": f"http://127.0.0.1:{server['port']}/",
                 "token": TEST_TOKEN,
                 # backoff-factor=0 keeps the test fast; retries=3 covers three 503s + the eventual 200.
-                CONNECTION: {CONNECTION_RETRIES: 3, CONNECTION_BACKOFF_FACTOR: 0},
+                REST_CLIENT_MAX_RETRIES: 3,
+                REST_CLIENT_RETRY_BACKOFF_FACTOR: 0,
             },
         )
         assert catalog.list_namespaces() == [("foo",)]
@@ -2149,7 +2147,8 @@ def test_session_exhausted_retries_surfaces_typed_exception() -> None:
             **{  # type: ignore
                 "uri": f"http://127.0.0.1:{server['port']}/",
                 "token": TEST_TOKEN,
-                CONNECTION: {CONNECTION_RETRIES: 2, CONNECTION_BACKOFF_FACTOR: 0},
+                REST_CLIENT_MAX_RETRIES: 2,
+                REST_CLIENT_RETRY_BACKOFF_FACTOR: 0,
             },
         )
         with pytest.raises(ServiceUnavailableError):
@@ -2162,19 +2161,19 @@ def test_session_with_invalid_connection_timeout_raises(rest_mock: Mocker) -> No
     catalog_properties = {
         "uri": TEST_URI,
         "token": TEST_TOKEN,
-        CONNECTION: {CONNECTION_TIMEOUT: -1},
+        REST_CLIENT_REQUEST_TIMEOUT: -1,
     }
-    with pytest.raises(ValueError, match="`connection.timeout` must be a positive number"):
+    with pytest.raises(ValueError, match="`rest.client.request-timeout` must be a positive number"):
         RestCatalog("rest", **catalog_properties)  # type: ignore
 
 
 def test_session_with_invalid_connection_retries_raises(rest_mock: Mocker) -> None:
     catalog_properties = {
         "uri": TEST_URI,
         "token": TEST_TOKEN,
-        CONNECTION: {CONNECTION_RETRIES: -1},
+        REST_CLIENT_MAX_RETRIES: -1,
     }
-    with pytest.raises(ValueError, match="`connection.retries` must be non-negative"):
+    with pytest.raises(ValueError, match="`rest.client.max-retries` must be non-negative"):
         RestCatalog("rest", **catalog_properties)  # type: ignore