danielcweeks commented on code in PR #3418:
URL: https://github.com/apache/iceberg-python/pull/3418#discussion_r3461666606
##########
pyiceberg/catalog/rest/__init__.py:
##########
@@ -396,6 +403,87 @@ 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,
+ 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: Mapping[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:
+ """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}")
+
+ # `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}")
+
+ return _RetryTimeoutHTTPAdapter(
Review Comment:
I think there's a pretty significant issue here. The `Retry` policy will
raise a `RetryError` instead of returning the failure code. This means the
exception mapping is then broken which impacts how we handle error codes.
--
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]
