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

Author: rahulsmahadev

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:

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):

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: