REST: Add retry and timeout configuration for REST catalog by rahulsmahadev · Pull Request #3418 · apache/iceberg-python

rahulsmahadev · 2026-05-26T23:25:47Z

Rationale for this change

The REST Catalog uses requests with no retries and no timeout configured, so transient 5xx / network failures bubble up immediately and slow servers can hang the client indefinitely. The reporting user on #2772 hit infinite-timeout hangs against a Polaris instance returning 504 from a proxy.

This change adds an optional connection: block on the catalog properties for opting in to a per-request timeout and a retry policy, as proposed in the issue body:

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]

Changes:

Added connection, connection.timeout, connection.retry catalog properties.
Added a private _RetryTimeoutHTTPAdapter that subclasses requests.adapters.HTTPAdapter and applies a default timeout to every call (since requests does not expose a session-wide timeout).
The connection.retry sub-mapping is passed verbatim to urllib3.util.retry.Retry, so any kwarg the upstream class accepts can be configured.
Both keys are optional and opt-in. When neither is set, the default requests behavior is preserved — no behavior change for existing users.
Validation: negative or zero timeout raises ValueError; unknown Retry kwargs are caught and re-raised as ValueError with a clearer message.

The SigV4 adapter mounted at the catalog URI (see #3063) is a longer-prefix match and still wins for that host, so SigV4 users continue to get their existing retry behavior unchanged — this change deliberately stays out of that path.

Are these changes tested?

Yes. Added five tests in tests/catalog/test_rest.py:

test_session_without_connection_config_uses_default_adapter — no connection: block → no _RetryTimeoutHTTPAdapter is mounted (default requests behavior preserved).
test_session_with_connection_timeout_and_retry — full nested connection: block applies timeout and all Retry kwargs (total, backoff_factor, status_forcelist, allowed_methods).
test_session_with_connection_timeout_only — timeout alone works without a retry block.
test_session_with_invalid_connection_timeout_raises — negative timeout raises ValueError.
test_session_with_invalid_connection_retry_kwarg_raises — unknown Retry kwarg raises ValueError.

Are there any user-facing changes?

Yes — two new optional REST catalog properties (connection.timeout, connection.retry) documented in mkdocs/docs/configuration.md under "Retry and timeout". No behavior change for users who do not set them.

Closes apache#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 <rahul.mahadev@databricks.com>

rambleraptor · 2026-05-27T01:57:15Z

Can you take a look at the linter failures?

'make lint' will help you run the linters locally.

Fokko · 2026-05-27T07:06:29Z

+| 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). |


I feel like having a dictionary here is a bit tricky; this couples the configuration tightly with urllib3. When urllib changes something, or we want to use another library, then we have to map this. How about adding the options explicitly.

+1 on this.

Allowing so many retry options can also lead to unintended behaviors. As an example, what happens if you set raise_on_status to False, but you receive a 403 that you should raise an error on?

I think we should focus on timeout, # of retries, and a backoff factor.

Good call — dropped the dict pass-through. The connection block now exposes only three explicit options: timeout, retries, and backoff-factor. The status_forcelist and allowed_methods are hard-coded internally to transient codes (429/500/502/503/504) and idempotent methods (GET/HEAD/OPTIONS), so users can't accidentally swallow non-transient errors or retry non-idempotent writes. Fixed in 6fb87ff.

rambleraptor · 2026-05-27T23:01:40Z

    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:


Can we get a test where we set the retry logic and then see the retries occur? We should be able to simulate this with mock HTTP calls and then see that X number of HTTP calls were made afterwards.

Added test_session_retries_on_transient_5xx_then_succeeds in 6fb87ff. requests_mock actually replaces the HTTPAdapter on the session, which bypasses our retry logic, so the test instead stands up a real http.server on a loopback port. The handler returns three 503s followed by a 200, and the test asserts both that list_namespaces succeeds and that the handler saw 4 requests.

@Fokko

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 <rahul.mahadev@databricks.com>

rahulsmahadev · 2026-06-09T22:10:30Z

Pushed 6fb87ff addressing the review feedback:

@Fokko / @rambleraptor: dropped the urllib3 retry-dict pass-through. The connection block now exposes only three explicit options — timeout, retries, backoff-factor — with status_forcelist and allowed_methods hard-coded internally (transient codes + idempotent verbs), so users can't accidentally swallow non-transient errors or retry non-idempotent writes.
@rambleraptor (retry test): added test_session_retries_on_transient_5xx_then_succeeds that uses a real loopback http.server (since requests_mock replaces the HTTPAdapter and bypasses retry logic). The handler returns three 503s then a 200; the test asserts both that list_namespaces succeeds and that the handler saw 4 requests.
@rambleraptor (lint): the three CI mypy errors are fixed — _RetryTimeoutHTTPAdapter.send now matches the parent signature, and the set(allowed_methods) call guards the Collection[str] | None type.

PTAL!

rambleraptor · 2026-06-09T22:24:17Z

Can you run make lint and get the linters passing? I'll take another look after that. Thanks!

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 <rahul.mahadev@databricks.com>

rahulsmahadev · 2026-06-10T00:08:10Z

Linters are now passing — afb1f51 fixed the last mypy holdouts: I'd used MutableMapping for the proxies parameter on the overridden HTTPAdapter.send, but the parent (per types-requests) declares it as Mapping, and overriding with a more specific type is an invalid variance under strict mypy. Switched to Mapping. All 17 checks green now. PTAL!

rambleraptor

A couple quick notes. This is basically there in my opinion. Thanks a lot for doing all of this!

rambleraptor · 2026-06-10T21:09:15Z

+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.


I'd get rid of this sentence.

If you override connection.timeout, connections.retries should still take its default value. This sentence makes it sound like you have to specify the default values on the other places or else you'll have undefined behavior.

Dropped that sentence in 47a5382.

rambleraptor · 2026-06-10T21:18:06Z

+    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,


We can set retries initially to 0 to avoid this statement.

Good call — initialized retries and backoff_factor to 0 in 47a5382, so the inline conditional defaults are gone. A Retry(total=0, backoff_factor=0) is a no-op, functionally equivalent to no Retry at all.

rambleraptor · 2026-06-10T21:18:11Z

+    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,


Same as above.

Same fix — backoff_factor now initializes to 0.0 alongside retries. See reply above.

rambleraptor · 2026-06-10T21:19:42Z

+        {"defaults": {}, "overrides": {}, "endpoints": [str(endpoint) for endpoint in TEST_SUPPORTED_ENDPOINTS]}
+    ).encode()
+
+    class _Handler(BaseHTTPRequestHandler):


Can you add the server setup into a different method? That way, we can easily see what this is actually testing + less about the test setup.

Done in 47a5382 — extracted the handler + threading setup into a _local_rest_server_503_then_200(num_failures) context manager. The test body is now just the catalog construction, list_namespaces(), and the two assertions.

- 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 <rahul.mahadev@databricks.com>

rahulsmahadev · 2026-06-11T21:55:59Z

Pushed 47a5382 addressing the four review nits:

Dropped the misleading "when none are set" sentence from the docs.
Initialized retries and backoff_factor to 0 so the Retry(...) call no longer needs inline conditional defaults. A Retry(total=0) is a no-op.
Extracted the loopback HTTP server scaffold from the retry test into a _local_rest_server_503_then_200 context manager; the test body is now just the catalog construction + assertions.

Thanks again for the careful review!

rambleraptor

Thank you for doing this! This will be super helpful.

Fokko reviewed May 27, 2026

View reviewed changes

rambleraptor reviewed May 27, 2026

View reviewed changes

rahulsmahadev requested review from Fokko and rambleraptor June 10, 2026 01:02

rambleraptor reviewed Jun 10, 2026

View reviewed changes

rambleraptor approved these changes Jun 11, 2026

View reviewed changes

		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:

Conversation

rahulsmahadev commented May 26, 2026

Rationale for this change

Are these changes tested?

Are there any user-facing changes?

Uh oh!

rambleraptor commented May 27, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rahulsmahadev commented Jun 9, 2026

Uh oh!

rambleraptor commented Jun 9, 2026

Uh oh!

rahulsmahadev commented Jun 10, 2026

Uh oh!

rambleraptor left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rahulsmahadev commented Jun 11, 2026

Uh oh!

rambleraptor left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants