From 9d85042b0fec350ebefd5cb7f626d315a95bf522 Mon Sep 17 00:00:00 2001 From: rahulsmahadev Date: Tue, 26 May 2026 23:20:28 +0000 Subject: [PATCH 1/7] REST: Add retry and timeout configuration for REST catalog Closes #2772. 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 (e.g. a Polaris instance returning 504 from a proxy). Add an optional connection: block on the catalog properties: catalog: default: uri: http://rest-catalog/ws/ connection: timeout: 60 retry: total: 5 backoff_factor: 1.0 status_forcelist: [429, 500, 502, 503, 504] allowed_methods: [GET, HEAD, OPTIONS] connection.retry is passed verbatim to urllib3.util.retry.Retry. Both keys are optional and opt-in: when neither is set the default requests behavior is preserved. Signed-off-by: rahulsmahadev --- mkdocs/docs/configuration.md | 25 +++++++++++ pyiceberg/catalog/rest/__init__.py | 72 ++++++++++++++++++++++++++++-- tests/catalog/test_rest.py | 69 ++++++++++++++++++++++++++++ 3 files changed, 163 insertions(+), 3 deletions(-) diff --git a/mkdocs/docs/configuration.md b/mkdocs/docs/configuration.md index 3f82f78895..fbb8265f54 100644 --- a/mkdocs/docs/configuration.md +++ b/mkdocs/docs/configuration.md @@ -348,6 +348,31 @@ catalog: | snapshot-loading-mode | refs | The snapshots to return in the body of the metadata. Setting the value to `all` would return the full set of snapshots currently valid for the table. Setting the value to `refs` would load all snapshots referenced by branches or tags. | | `header.X-Iceberg-Access-Delegation` | `vended-credentials` | Signal to the server that the client supports delegated access via a comma-separated list of access mechanisms. The server may choose to supply access via any or none of the requested mechanisms. When using `vended-credentials`, the server provides temporary credentials to the client. When using `remote-signing`, the server signs requests on behalf of the client. (default: `vended-credentials`) | +#### Retry and timeout + +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. +Both keys are optional; when neither is set, the default `requests` behavior is preserved. + +```yaml +catalog: + default: + uri: http://rest-catalog/ws/ + connection: + timeout: 60 # seconds, applied to every HTTP call + retry: + total: 5 + backoff_factor: 1.0 + status_forcelist: [429, 500, 502, 503, 504] + allowed_methods: [GET, HEAD, OPTIONS] +``` + +| Key | Example | Description | +| ---------------------------- | ------------------------------------ | ------------------------------------------------------------------------------------------------------ | +| connection.timeout | 60 | Per-request timeout in seconds. Must be a positive number. | +| connection.retry | `{total: 5, backoff_factor: 1.0}` | Mapping passed verbatim as kwargs to [`urllib3.util.retry.Retry`](https://urllib3.readthedocs.io/en/latest/reference/urllib3.util.html#urllib3.util.Retry). | + #### Headers in REST Catalog To configure custom headers in REST Catalog, include them in the catalog properties with `header.`. This diff --git a/pyiceberg/catalog/rest/__init__.py b/pyiceberg/catalog/rest/__init__.py index 39954ef561..e0457e17e7 100644 --- a/pyiceberg/catalog/rest/__init__.py +++ b/pyiceberg/catalog/rest/__init__.py @@ -25,9 +25,11 @@ from urllib.parse import quote, unquote from pydantic import ConfigDict, Field, TypeAdapter, field_validator -from requests import HTTPError, Session +from requests import HTTPError, PreparedRequest, Response, Session +from requests.adapters import HTTPAdapter from tenacity import RetryCallState, retry, retry_if_exception_type, stop_after_attempt from typing_extensions import override +from urllib3.util.retry import Retry from pyiceberg import __version__ from pyiceberg.catalog import BOTOCORE_SESSION, TOKEN, URI, WAREHOUSE_LOCATION, Catalog, PropertiesUpdateSummary @@ -255,6 +257,9 @@ 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_RETRY = "retry" EMPTY_BODY_SHA256: str = "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855" OAUTH2_SERVER_URI = "oauth2-server-uri" SNAPSHOT_LOADING_MODE = "snapshot-loading-mode" @@ -392,6 +397,63 @@ class ListViewsResponse(IcebergBaseModel): _PLANNING_RESPONSE_ADAPTER = TypeAdapter(PlanningResponse) +class _RetryTimeoutHTTPAdapter(HTTPAdapter): + """HTTPAdapter that applies a default per-request timeout. + + requests does not provide a way to set a default timeout on a Session; + without this adapter, every call would have to thread `timeout=` through. + 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: + self._timeout = timeout + if max_retries is not None: + super().__init__(max_retries=max_retries) + else: + super().__init__() + + def send(self, request: PreparedRequest, **kwargs: Any) -> Response: + if kwargs.get("timeout") is None and self._timeout is not None: + kwargs["timeout"] = self._timeout + return super().send(request, **kwargs) + + +def _create_connection_adapter(properties: Properties) -> _RetryTimeoutHTTPAdapter | None: + """Build a connection adapter from the optional `connection.*` properties. + + Returns None when no `connection` block is supplied, leaving the default + Session behavior unchanged. Raises ValueError on invalid input. + """ + connection_config = properties.get(CONNECTION) + if not connection_config: + 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}") + + retry: Retry | None = None + if (retry_config := connection_config.get(CONNECTION_RETRY)) is not None: + if not isinstance(retry_config, dict): + raise ValueError(f"`{CONNECTION}.{CONNECTION_RETRY}` must be a mapping, got: {type(retry_config).__name__}") + try: + retry = Retry(**retry_config) + except TypeError as e: + raise ValueError(f"Invalid `{CONNECTION}.{CONNECTION_RETRY}` configuration: {e}") from e + + if timeout is None and retry is None: + return None + + return _RetryTimeoutHTTPAdapter(timeout=timeout, max_retries=retry) + + class RestCatalog(Catalog): uri: str _session: Session @@ -418,6 +480,12 @@ def _create_session(self) -> Session: """Create a request session with provided catalog configuration.""" session = Session() + # Mount the retry/timeout adapter when `connection.*` properties are set. + # SigV4's adapter mounted below at `self.uri` is a longer prefix and still wins for that host. + if (connection_adapter := _create_connection_adapter(self.properties)) is not None: + session.mount("http://", connection_adapter) + session.mount("https://", connection_adapter) + # Set HTTP headers self._config_headers(session) @@ -763,8 +831,6 @@ def _init_sigv4(self, session: Session) -> None: import boto3 from botocore.auth import SigV4Auth from botocore.awsrequest import AWSRequest - from requests import PreparedRequest - from requests.adapters import HTTPAdapter class SigV4Adapter(HTTPAdapter): def __init__(self, **properties: str): diff --git a/tests/catalog/test_rest.py b/tests/catalog/test_rest.py index df2f96a392..978ce40777 100644 --- a/tests/catalog/test_rest.py +++ b/tests/catalog/test_rest.py @@ -32,6 +32,9 @@ import pyiceberg from pyiceberg.catalog import PropertiesUpdateSummary, load_catalog from pyiceberg.catalog.rest import ( + CONNECTION, + CONNECTION_RETRY, + CONNECTION_TIMEOUT, DEFAULT_ENDPOINTS, EMPTY_BODY_SHA256, OAUTH2_SERVER_URI, @@ -43,6 +46,7 @@ HttpMethod, RestCatalog, ScanPlanningMode, + _RetryTimeoutHTTPAdapter, ) from pyiceberg.exceptions import ( AuthorizationExpiredError, @@ -2019,6 +2023,71 @@ def test_request_session_with_ssl_client_cert() -> None: assert "Could not find the TLS certificate file, invalid path: path_to_client_cert" in str(e.value) +def test_session_without_connection_config_uses_default_adapter(rest_mock: Mocker) -> None: + catalog = RestCatalog("rest", uri=TEST_URI, token=TEST_TOKEN) + for adapter in catalog._session.adapters.values(): + assert not isinstance(adapter, _RetryTimeoutHTTPAdapter) + + +def test_session_with_connection_timeout_and_retry(rest_mock: Mocker) -> None: + catalog_properties = { + "uri": TEST_URI, + "token": TEST_TOKEN, + CONNECTION: { + CONNECTION_TIMEOUT: 60, + CONNECTION_RETRY: { + "total": 5, + "backoff_factor": 1.0, + "status_forcelist": [429, 500, 502, 503, 504], + "allowed_methods": ["GET", "HEAD", "OPTIONS"], + }, + }, + } + catalog = RestCatalog("rest", **catalog_properties) # type: ignore + + https_adapter = catalog._session.adapters["https://"] + http_adapter = catalog._session.adapters["http://"] + assert isinstance(https_adapter, _RetryTimeoutHTTPAdapter) + assert https_adapter is http_adapter + assert https_adapter._timeout == 60.0 + assert https_adapter.max_retries.total == 5 + assert https_adapter.max_retries.backoff_factor == 1.0 + assert https_adapter.max_retries.status_forcelist == [429, 500, 502, 503, 504] + assert set(https_adapter.max_retries.allowed_methods) == {"GET", "HEAD", "OPTIONS"} + + +def test_session_with_connection_timeout_only(rest_mock: Mocker) -> None: + catalog_properties = { + "uri": TEST_URI, + "token": TEST_TOKEN, + CONNECTION: {CONNECTION_TIMEOUT: "30"}, + } + catalog = RestCatalog("rest", **catalog_properties) # type: ignore + adapter = catalog._session.adapters["https://"] + assert isinstance(adapter, _RetryTimeoutHTTPAdapter) + assert adapter._timeout == 30.0 + + +def test_session_with_invalid_connection_timeout_raises(rest_mock: Mocker) -> None: + catalog_properties = { + "uri": TEST_URI, + "token": TEST_TOKEN, + CONNECTION: {CONNECTION_TIMEOUT: -1}, + } + with pytest.raises(ValueError, match="`connection.timeout` must be a positive number"): + RestCatalog("rest", **catalog_properties) # type: ignore + + +def test_session_with_invalid_connection_retry_kwarg_raises(rest_mock: Mocker) -> None: + catalog_properties = { + "uri": TEST_URI, + "token": TEST_TOKEN, + CONNECTION: {CONNECTION_RETRY: {"bogus_kwarg": 1}}, + } + with pytest.raises(ValueError, match="Invalid `connection.retry` configuration"): + RestCatalog("rest", **catalog_properties) # type: ignore + + def test_rest_catalog_with_basic_auth_type(rest_mock: Mocker) -> None: # Given rest_mock.get( From 6fb87ffe966bb4116fe54494f38f0444ebe4a41a Mon Sep 17 00:00:00 2001 From: rahulsmahadev Date: Tue, 9 Jun 2026 22:00:14 +0000 Subject: [PATCH 2/7] Address review feedback: explicit options, exercise retries, fix mypy Per @Fokko + @rambleraptor: drop the urllib3 retry-dict pass-through and expose only three explicit knobs on the connection block (timeout, retries, backoff-factor). Hard-code the retry policy (status_forcelist of transient codes; allowed_methods of idempotent verbs) so users cannot misconfigure e.g. raise_on_status=False and silently swallow 4xx errors. Per @rambleraptor: add a test that exercises the retry path end-to-end by spinning up a loopback HTTP server that returns three 503s then a 200, verifying the catalog makes all four attempts. requests_mock can't be used here because it replaces the HTTPAdapter and bypasses retry logic. Fix the three mypy errors flagged by CI: - _RetryTimeoutHTTPAdapter.send now matches HTTPAdapter.send's full signature instead of (request, **kwargs). - Test's set(adapter.max_retries.allowed_methods) now guards the Collection[str] | None type. Signed-off-by: rahulsmahadev --- mkdocs/docs/configuration.md | 29 +++++----- pyiceberg/catalog/rest/__init__.py | 60 ++++++++++++++++----- tests/catalog/test_rest.py | 85 +++++++++++++++++++++++++----- 3 files changed, 134 insertions(+), 40 deletions(-) diff --git a/mkdocs/docs/configuration.md b/mkdocs/docs/configuration.md index fbb8265f54..4ad6f8b1c7 100644 --- a/mkdocs/docs/configuration.md +++ b/mkdocs/docs/configuration.md @@ -351,27 +351,28 @@ catalog: #### Retry and timeout 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. +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. -Both keys are optional; when neither is set, the default `requests` behavior is preserved. +Every key is optional; when none are set, the default `requests` behavior is preserved. ```yaml catalog: default: uri: http://rest-catalog/ws/ connection: - timeout: 60 # seconds, applied to every HTTP call - retry: - total: 5 - backoff_factor: 1.0 - status_forcelist: [429, 500, 502, 503, 504] - allowed_methods: [GET, HEAD, OPTIONS] -``` - -| Key | Example | Description | -| ---------------------------- | ------------------------------------ | ------------------------------------------------------------------------------------------------------ | -| connection.timeout | 60 | Per-request timeout in seconds. Must be a positive number. | -| connection.retry | `{total: 5, backoff_factor: 1.0}` | Mapping passed verbatim as kwargs to [`urllib3.util.retry.Retry`](https://urllib3.readthedocs.io/en/latest/reference/urllib3.util.html#urllib3.util.Retry). | + 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 +``` + +| 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. | + +Retries are applied to idempotent methods only (`GET`, `HEAD`, `OPTIONS`) and to the +transient HTTP status codes `429`, `500`, `502`, `503`, `504`. Other failures are not retried. #### Headers in REST Catalog diff --git a/pyiceberg/catalog/rest/__init__.py b/pyiceberg/catalog/rest/__init__.py index e0457e17e7..edf07df671 100644 --- a/pyiceberg/catalog/rest/__init__.py +++ b/pyiceberg/catalog/rest/__init__.py @@ -17,6 +17,7 @@ from __future__ import annotations from collections import deque +from collections.abc import MutableMapping from enum import Enum from typing import ( TYPE_CHECKING, @@ -259,7 +260,12 @@ class ScanPlanningMode(Enum): SIGV4_MAX_RETRIES_DEFAULT = 10 CONNECTION = "connection" CONNECTION_TIMEOUT = "timeout" -CONNECTION_RETRY = "retry" +CONNECTION_RETRIES = "retries" +CONNECTION_BACKOFF_FACTOR = "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"}) EMPTY_BODY_SHA256: str = "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855" OAUTH2_SERVER_URI = "oauth2-server-uri" SNAPSHOT_LOADING_MODE = "snapshot-loading-mode" @@ -412,10 +418,18 @@ def __init__(self, timeout: float | None = None, max_retries: Retry | int | None else: super().__init__() - def send(self, request: PreparedRequest, **kwargs: Any) -> Response: - if kwargs.get("timeout") is None and self._timeout is not None: - kwargs["timeout"] = self._timeout - return super().send(request, **kwargs) + def send( + self, + request: PreparedRequest, + stream: bool = False, + timeout: None | float | tuple[float, float] | tuple[float, None] = None, + verify: bool | str = True, + cert: None | bytes | str | tuple[bytes | str, bytes | str] = None, + proxies: MutableMapping[str, str] | None = None, + ) -> Response: + if timeout is None: + timeout = self._timeout + return super().send(request, stream=stream, timeout=timeout, verify=verify, cert=cert, proxies=proxies) def _create_connection_adapter(properties: Properties) -> _RetryTimeoutHTTPAdapter | None: @@ -439,19 +453,37 @@ def _create_connection_adapter(properties: Properties) -> _RetryTimeoutHTTPAdapt if timeout <= 0: raise ValueError(f"`{CONNECTION}.{CONNECTION_TIMEOUT}` must be a positive number, got: {timeout}") - retry: Retry | None = None - if (retry_config := connection_config.get(CONNECTION_RETRY)) is not None: - if not isinstance(retry_config, dict): - raise ValueError(f"`{CONNECTION}.{CONNECTION_RETRY}` must be a mapping, got: {type(retry_config).__name__}") + retries: int | None = None + 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: float | None = None + if (raw_backoff := connection_config.get(CONNECTION_BACKOFF_FACTOR)) is not None: try: - retry = Retry(**retry_config) - except TypeError as e: - raise ValueError(f"Invalid `{CONNECTION}.{CONNECTION_RETRY}` configuration: {e}") from e + 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}") + + max_retries: Retry | None = None + if retries is not None or backoff_factor is not None: + max_retries = Retry( + total=retries if retries is not None else 0, + backoff_factor=backoff_factor if backoff_factor is not None else 0, + status_forcelist=list(_CONNECTION_RETRY_STATUS_FORCELIST), + allowed_methods=_CONNECTION_RETRY_ALLOWED_METHODS, + ) - if timeout is None and retry is None: + if timeout is None and max_retries is None: return None - return _RetryTimeoutHTTPAdapter(timeout=timeout, max_retries=retry) + return _RetryTimeoutHTTPAdapter(timeout=timeout, max_retries=max_retries) class RestCatalog(Catalog): diff --git a/tests/catalog/test_rest.py b/tests/catalog/test_rest.py index 978ce40777..6edeb1cadf 100644 --- a/tests/catalog/test_rest.py +++ b/tests/catalog/test_rest.py @@ -33,7 +33,8 @@ from pyiceberg.catalog import PropertiesUpdateSummary, load_catalog from pyiceberg.catalog.rest import ( CONNECTION, - CONNECTION_RETRY, + CONNECTION_BACKOFF_FACTOR, + CONNECTION_RETRIES, CONNECTION_TIMEOUT, DEFAULT_ENDPOINTS, EMPTY_BODY_SHA256, @@ -2029,18 +2030,14 @@ def test_session_without_connection_config_uses_default_adapter(rest_mock: Mocke assert not isinstance(adapter, _RetryTimeoutHTTPAdapter) -def test_session_with_connection_timeout_and_retry(rest_mock: Mocker) -> None: +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_RETRY: { - "total": 5, - "backoff_factor": 1.0, - "status_forcelist": [429, 500, 502, 503, 504], - "allowed_methods": ["GET", "HEAD", "OPTIONS"], - }, + CONNECTION_RETRIES: 5, + CONNECTION_BACKOFF_FACTOR: 1.0, }, } catalog = RestCatalog("rest", **catalog_properties) # type: ignore @@ -2052,8 +2049,10 @@ def test_session_with_connection_timeout_and_retry(rest_mock: Mocker) -> None: assert https_adapter._timeout == 60.0 assert https_adapter.max_retries.total == 5 assert https_adapter.max_retries.backoff_factor == 1.0 + # Internal retry policy: transient codes and idempotent methods only. assert https_adapter.max_retries.status_forcelist == [429, 500, 502, 503, 504] - assert set(https_adapter.max_retries.allowed_methods) == {"GET", "HEAD", "OPTIONS"} + allowed_methods = https_adapter.max_retries.allowed_methods or frozenset() + assert set(allowed_methods) == {"GET", "HEAD", "OPTIONS"} def test_session_with_connection_timeout_only(rest_mock: Mocker) -> None: @@ -2066,6 +2065,68 @@ def test_session_with_connection_timeout_only(rest_mock: Mocker) -> None: adapter = catalog._session.adapters["https://"] assert isinstance(adapter, _RetryTimeoutHTTPAdapter) assert adapter._timeout == 30.0 + # No retry options set, so no Retry object is configured. + assert adapter.max_retries.total == 0 + + +def test_session_retries_on_transient_5xx_then_succeeds() -> None: + """Three real 503 responses followed by a 200; the catalog should make all four attempts. + + `requests_mock` would replace our HTTPAdapter, bypassing the retry logic we want to exercise, + so this test stands up an actual `http.server` on a loopback port instead. + """ + import json + import threading + from http.server import BaseHTTPRequestHandler, HTTPServer + + state = {"namespace_calls": 0} + config_body = json.dumps( + {"defaults": {}, "overrides": {}, "endpoints": [str(endpoint) for endpoint in TEST_SUPPORTED_ENDPOINTS]} + ).encode() + + class _Handler(BaseHTTPRequestHandler): + def do_GET(self) -> None: + if self.path.endswith("/v1/config"): + self._respond(200, config_body) + elif self.path.endswith("/v1/namespaces"): + state["namespace_calls"] += 1 + if state["namespace_calls"] <= 3: + self._respond(503, b"") + else: + self._respond(200, json.dumps({"namespaces": [["foo"]]}).encode()) + else: + self._respond(404, b"") + + def _respond(self, status: int, body: bytes) -> None: + self.send_response(status) + self.send_header("Content-Type", "application/json") + self.send_header("Content-Length", str(len(body))) + self.end_headers() + if body: + self.wfile.write(body) + + def log_message(self, format: str, *args: Any) -> None: # silence default access logs + pass + + server = HTTPServer(("127.0.0.1", 0), _Handler) + port = server.server_address[1] + server_thread = threading.Thread(target=server.serve_forever, daemon=True) + server_thread.start() + try: + catalog = RestCatalog( + "rest", + **{ # type: ignore + "uri": f"http://127.0.0.1:{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}, + }, + ) + assert catalog.list_namespaces() == [("foo",)] + assert state["namespace_calls"] == 4 + finally: + server.shutdown() + server.server_close() def test_session_with_invalid_connection_timeout_raises(rest_mock: Mocker) -> None: @@ -2078,13 +2139,13 @@ def test_session_with_invalid_connection_timeout_raises(rest_mock: Mocker) -> No RestCatalog("rest", **catalog_properties) # type: ignore -def test_session_with_invalid_connection_retry_kwarg_raises(rest_mock: Mocker) -> None: +def test_session_with_invalid_connection_retries_raises(rest_mock: Mocker) -> None: catalog_properties = { "uri": TEST_URI, "token": TEST_TOKEN, - CONNECTION: {CONNECTION_RETRY: {"bogus_kwarg": 1}}, + CONNECTION: {CONNECTION_RETRIES: -1}, } - with pytest.raises(ValueError, match="Invalid `connection.retry` configuration"): + with pytest.raises(ValueError, match="`connection.retries` must be non-negative"): RestCatalog("rest", **catalog_properties) # type: ignore From afb1f51004c36fe92fe77a5ae6ce95f51d4ef780 Mon Sep 17 00:00:00 2001 From: rahulsmahadev Date: Tue, 9 Jun 2026 23:22:42 +0000 Subject: [PATCH 3/7] Fix mypy: proxies type must be Mapping, not MutableMapping requests' HTTPAdapter.send declares proxies as Mapping[str, str] | None (via types-requests). Overriding with the more specific MutableMapping is an invalid override under mypy 1.18 strict mode. Signed-off-by: rahulsmahadev --- pyiceberg/catalog/rest/__init__.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pyiceberg/catalog/rest/__init__.py b/pyiceberg/catalog/rest/__init__.py index edf07df671..97143e7a89 100644 --- a/pyiceberg/catalog/rest/__init__.py +++ b/pyiceberg/catalog/rest/__init__.py @@ -17,7 +17,7 @@ from __future__ import annotations from collections import deque -from collections.abc import MutableMapping +from collections.abc import Mapping from enum import Enum from typing import ( TYPE_CHECKING, @@ -425,7 +425,7 @@ def send( timeout: None | float | tuple[float, float] | tuple[float, None] = None, verify: bool | str = True, cert: None | bytes | str | tuple[bytes | str, bytes | str] = None, - proxies: MutableMapping[str, str] | None = None, + proxies: Mapping[str, str] | None = None, ) -> Response: if timeout is None: timeout = self._timeout From 47a5382282fc215add98f8ed05ec8ddc0553d8b9 Mon Sep 17 00:00:00 2001 From: rahulsmahadev Date: Thu, 11 Jun 2026 21:55:35 +0000 Subject: [PATCH 4/7] Address review nits from @rambleraptor - Drop the misleading 'when none are set, the default requests behavior is preserved' sentence in the docs (partial overrides don't reset other knobs to undefined behavior). - Initialize retries and backoff_factor to 0 instead of None so the Retry() call no longer needs conditional defaults inline. A Retry(total=0) is a no-op and is functionally equivalent to no Retry at all. - Extract the loopback HTTP server setup from the retry test into a _local_rest_server_503_then_200 context manager; the test body now shows intent (set retries=3, list, verify 4 calls) without the handler scaffold. Signed-off-by: rahulsmahadev --- mkdocs/docs/configuration.md | 1 - pyiceberg/catalog/rest/__init__.py | 24 +++++++++---------- tests/catalog/test_rest.py | 37 +++++++++++++++++++----------- 3 files changed, 34 insertions(+), 28 deletions(-) diff --git a/mkdocs/docs/configuration.md b/mkdocs/docs/configuration.md index 4ad6f8b1c7..41a8889eb9 100644 --- a/mkdocs/docs/configuration.md +++ b/mkdocs/docs/configuration.md @@ -353,7 +353,6 @@ 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. -Every key is optional; when none are set, the default `requests` behavior is preserved. ```yaml catalog: diff --git a/pyiceberg/catalog/rest/__init__.py b/pyiceberg/catalog/rest/__init__.py index 97143e7a89..feb7c1d7bb 100644 --- a/pyiceberg/catalog/rest/__init__.py +++ b/pyiceberg/catalog/rest/__init__.py @@ -453,7 +453,9 @@ def _create_connection_adapter(properties: Properties) -> _RetryTimeoutHTTPAdapt if timeout <= 0: raise ValueError(f"`{CONNECTION}.{CONNECTION_TIMEOUT}` must be a positive number, got: {timeout}") - retries: int | None = None + # `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) @@ -462,7 +464,7 @@ def _create_connection_adapter(properties: Properties) -> _RetryTimeoutHTTPAdapt if retries < 0: raise ValueError(f"`{CONNECTION}.{CONNECTION_RETRIES}` must be non-negative, got: {retries}") - backoff_factor: float | None = None + backoff_factor = 0.0 if (raw_backoff := connection_config.get(CONNECTION_BACKOFF_FACTOR)) is not None: try: backoff_factor = float(raw_backoff) @@ -471,19 +473,15 @@ def _create_connection_adapter(properties: Properties) -> _RetryTimeoutHTTPAdapt if backoff_factor < 0: raise ValueError(f"`{CONNECTION}.{CONNECTION_BACKOFF_FACTOR}` must be non-negative, got: {backoff_factor}") - max_retries: Retry | None = None - if retries is not None or backoff_factor is not None: - max_retries = Retry( - total=retries if retries is not None else 0, - backoff_factor=backoff_factor if backoff_factor is not None else 0, + return _RetryTimeoutHTTPAdapter( + timeout=timeout, + max_retries=Retry( + total=retries, + backoff_factor=backoff_factor, status_forcelist=list(_CONNECTION_RETRY_STATUS_FORCELIST), allowed_methods=_CONNECTION_RETRY_ALLOWED_METHODS, - ) - - if timeout is None and max_retries is None: - return None - - return _RetryTimeoutHTTPAdapter(timeout=timeout, max_retries=max_retries) + ), + ) class RestCatalog(Catalog): diff --git a/tests/catalog/test_rest.py b/tests/catalog/test_rest.py index 6edeb1cadf..f038c08d20 100644 --- a/tests/catalog/test_rest.py +++ b/tests/catalog/test_rest.py @@ -19,7 +19,8 @@ import base64 import os -from collections.abc import Callable +from collections.abc import Callable, Iterator +from contextlib import contextmanager from typing import Any, cast from unittest import mock @@ -2065,21 +2066,23 @@ def test_session_with_connection_timeout_only(rest_mock: Mocker) -> None: adapter = catalog._session.adapters["https://"] assert isinstance(adapter, _RetryTimeoutHTTPAdapter) assert adapter._timeout == 30.0 - # No retry options set, so no Retry object is configured. + # Default retry policy (total=0) is a no-op when only timeout is configured. assert adapter.max_retries.total == 0 -def test_session_retries_on_transient_5xx_then_succeeds() -> None: - """Three real 503 responses followed by a 200; the catalog should make all four attempts. +@contextmanager +def _local_rest_server_503_then_200(num_failures: int) -> Iterator[dict[str, Any]]: + """Stand up a loopback HTTP server that returns `num_failures` 503s for `/v1/namespaces` then a 200. + + Used in place of `requests_mock`, which replaces the HTTPAdapter and would bypass the retry logic. - `requests_mock` would replace our HTTPAdapter, bypassing the retry logic we want to exercise, - so this test stands up an actual `http.server` on a loopback port instead. + Yields a dict with `port` and `namespace_calls` keys (the latter is updated in-place as requests arrive). """ import json import threading from http.server import BaseHTTPRequestHandler, HTTPServer - state = {"namespace_calls": 0} + state: dict[str, Any] = {"namespace_calls": 0} config_body = json.dumps( {"defaults": {}, "overrides": {}, "endpoints": [str(endpoint) for endpoint in TEST_SUPPORTED_ENDPOINTS]} ).encode() @@ -2090,7 +2093,7 @@ def do_GET(self) -> None: self._respond(200, config_body) elif self.path.endswith("/v1/namespaces"): state["namespace_calls"] += 1 - if state["namespace_calls"] <= 3: + if state["namespace_calls"] <= num_failures: self._respond(503, b"") else: self._respond(200, json.dumps({"namespaces": [["foo"]]}).encode()) @@ -2109,24 +2112,30 @@ def log_message(self, format: str, *args: Any) -> None: # silence default acces pass server = HTTPServer(("127.0.0.1", 0), _Handler) - port = server.server_address[1] + state["port"] = server.server_address[1] server_thread = threading.Thread(target=server.serve_forever, daemon=True) server_thread.start() try: + yield state + finally: + server.shutdown() + server.server_close() + + +def test_session_retries_on_transient_5xx_then_succeeds() -> None: + """The catalog should retry on transient 5xx and succeed once the server stabilizes.""" + with _local_rest_server_503_then_200(num_failures=3) as server: catalog = RestCatalog( "rest", **{ # type: ignore - "uri": f"http://127.0.0.1:{port}/", + "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}, }, ) assert catalog.list_namespaces() == [("foo",)] - assert state["namespace_calls"] == 4 - finally: - server.shutdown() - server.server_close() + assert server["namespace_calls"] == 4 def test_session_with_invalid_connection_timeout_raises(rest_mock: Mocker) -> None: From 9a799c620ca1740c503251fcf41e9bf373628595 Mon Sep 17 00:00:00 2001 From: rahulsmahadev Date: Wed, 24 Jun 2026 00:16:14 +0000 Subject: [PATCH 5/7] Surface typed exceptions on retry exhaustion MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per @danielcweeks: the urllib3 Retry was raising MaxRetryError / RetryError on exhaustion, which short-circuited _handle_non_200_response and lost the typed exception mapping (ServiceUnavailableError for 503, etc.). Set raise_on_status=False so urllib3 returns the final 5xx response instead of raising; _handle_non_200_response then maps it to the typed exception as it does for non-retried responses. This is safe because status_forcelist is hard-coded to transient codes — 4xx codes are never retried and reach the mapping unchanged. Added a regression test that drives retries to exhaustion against a server that always returns 503 and asserts ServiceUnavailableError is raised (not RetryError). Signed-off-by: rahulsmahadev --- pyiceberg/catalog/rest/__init__.py | 5 +++++ tests/catalog/test_rest.py | 20 ++++++++++++++++++++ 2 files changed, 25 insertions(+) diff --git a/pyiceberg/catalog/rest/__init__.py b/pyiceberg/catalog/rest/__init__.py index feb7c1d7bb..d5f6580cc5 100644 --- a/pyiceberg/catalog/rest/__init__.py +++ b/pyiceberg/catalog/rest/__init__.py @@ -480,6 +480,11 @@ def _create_connection_adapter(properties: Properties) -> _RetryTimeoutHTTPAdapt backoff_factor=backoff_factor, status_forcelist=list(_CONNECTION_RETRY_STATUS_FORCELIST), allowed_methods=_CONNECTION_RETRY_ALLOWED_METHODS, + # Return the final response on retry exhaustion (instead of raising MaxRetryError) + # so `_handle_non_200_response` can map the 5xx status to a typed exception + # (ServiceUnavailableError, etc.). 4xx codes are not in status_forcelist and are + # never retried, so they reach the same mapping unchanged. + raise_on_status=False, ), ) diff --git a/tests/catalog/test_rest.py b/tests/catalog/test_rest.py index f038c08d20..23d6e3f0a6 100644 --- a/tests/catalog/test_rest.py +++ b/tests/catalog/test_rest.py @@ -60,6 +60,7 @@ NoSuchViewError, OAuthError, ServerError, + ServiceUnavailableError, TableAlreadyExistsError, ViewAlreadyExistsError, ) @@ -2138,6 +2139,25 @@ def test_session_retries_on_transient_5xx_then_succeeds() -> None: assert server["namespace_calls"] == 4 +def test_session_exhausted_retries_surfaces_typed_exception() -> None: + """When retries are exhausted, the typed exception from `_handle_non_200_response` should be raised + (e.g. `ServiceUnavailableError` for 503), not the urllib3 `MaxRetryError` / `RetryError`.""" + # `num_failures` greater than `retries + 1` guarantees the server never returns success. + with _local_rest_server_503_then_200(num_failures=100) as server: + catalog = RestCatalog( + "rest", + **{ # type: ignore + "uri": f"http://127.0.0.1:{server['port']}/", + "token": TEST_TOKEN, + CONNECTION: {CONNECTION_RETRIES: 2, CONNECTION_BACKOFF_FACTOR: 0}, + }, + ) + with pytest.raises(ServiceUnavailableError): + catalog.list_namespaces() + # retries=2 means 1 initial attempt + 2 retries = 3 calls + assert server["namespace_calls"] == 3 + + def test_session_with_invalid_connection_timeout_raises(rest_mock: Mocker) -> None: catalog_properties = { "uri": TEST_URI, From 36053c77a29efb54f1bd5b0093ebd93091660ac1 Mon Sep 17 00:00:00 2001 From: rahulsmahadev Date: Thu, 3 Sep 2026 21:26:07 +0000 Subject: [PATCH 6/7] 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. --- mkdocs/docs/configuration.md | 21 +++--- pyiceberg/catalog/rest/__init__.py | 115 +++++++++++++++++------------ tests/catalog/test_rest.py | 33 ++++----- 3 files changed, 93 insertions(+), 76 deletions(-) diff --git a/mkdocs/docs/configuration.md b/mkdocs/docs/configuration.md index 41a8889eb9..cb250fea1e 100644 --- a/mkdocs/docs/configuration.md +++ b/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 diff --git a/pyiceberg/catalog/rest/__init__.py b/pyiceberg/catalog/rest/__init__.py index d5f6580cc5..b09b77cfe4 100644 --- a/pyiceberg/catalog/rest/__init__.py +++ b/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) diff --git a/tests/catalog/test_rest.py b/tests/catalog/test_rest.py index 23d6e3f0a6..4fb0e6df10 100644 --- a/tests/catalog/test_rest.py +++ b/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,9 +2161,9 @@ 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 @@ -2172,9 +2171,9 @@ def test_session_with_invalid_connection_retries_raises(rest_mock: Mocker) -> No 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 From 6c52148fd7b0f5fa99863c4fd1b35a65860f6c4c Mon Sep 17 00:00:00 2001 From: rahulsmahadev Date: Sat, 5 Sep 2026 18:12:16 +0000 Subject: [PATCH 7/7] Fix mypy unused-ignore in REST client timeout test --- tests/catalog/test_rest.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/catalog/test_rest.py b/tests/catalog/test_rest.py index 3092dbe7ba..bf2d24b5e4 100644 --- a/tests/catalog/test_rest.py +++ b/tests/catalog/test_rest.py @@ -2325,7 +2325,7 @@ def test_session_with_connection_timeout_only(rest_mock: Mocker) -> None: "token": TEST_TOKEN, REST_CLIENT_REQUEST_TIMEOUT: "30", } - catalog = RestCatalog("rest", **catalog_properties) # type: ignore + catalog = RestCatalog("rest", **catalog_properties) adapter = catalog._session.adapters["https://"] assert isinstance(adapter, _RetryTimeoutHTTPAdapter) assert adapter._timeout == 30.0