rahulsmahadev commented on code in PR #3418: URL: https://github.com/apache/iceberg-python/pull/3418#discussion_r3384151983
########## 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). | Review Comment: 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. ########## tests/catalog/test_rest.py: ########## @@ -2287,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: Review Comment: 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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
