REST: Add retry and timeout configuration for REST catalog

[rahulsmahadev]

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.

[rambleraptor]

Can you take a look at the linter failures?

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

[Fokko]

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.

[rambleraptor]

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

[rahulsmahadev]

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]

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.

[rahulsmahadev]

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.

[rahulsmahadev]

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]

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

[rahulsmahadev]

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!