jiangxt2 commented on code in PR #14366:
URL: https://github.com/apache/gravitino/pull/14366#discussion_r4425355331
##########
clients/client-python/gravitino/exceptions/base.py:
##########
@@ -30,6 +30,27 @@ def __init__(self, message, *args):
super().__init__(message % args)
+class HTTPResponseException(RESTException):
+ """An HTTP failure whose response cannot be used as a domain error.
+
+ Attributes:
+ status_code: The HTTP status received from the server.
+ request_method: The original client request's HTTP method.
+ endpoint: The request origin, without credentials, path or query
parameters.
+ """
+
+ def __init__(
+ self, status_code: int, request_method: str, endpoint: str, reason: str
+ ):
+ """Create an error from safe context and a fixed diagnostic reason."""
+ self.status_code = status_code
+ self.request_method = request_method
+ self.endpoint = endpoint
+ super().__init__(
Review Comment:
Added explicit `__reduce__()` reconstruction from the safe reason and HTTP
context while preserving normal instance state. The pickle regression checks
type, message, `args` and context, and a real `spawn` process-pool test
verifies that `HTTPClient.get()` propagates the exception and that the same
pool can complete a subsequent successful request. Both pass on Python 3.12.9;
the pickle round-trip also passes on Python 3.10.18.
##########
clients/client-python/gravitino/utils/http_client.py:
##########
@@ -134,25 +139,84 @@ def _make_request(self, opener, request, timeout=None) ->
Tuple[bool, Response]:
try:
return (True, Response(opener.open(request, timeout=timeout)))
except HTTPError as err:
+ try:
+ return (False, self._read_error_response(err, request))
+ finally:
+ try:
+ err.close()
+ except OSError:
+ # Closing a failed response must not replace its HTTP
failure.
+ pass
+
+ def _read_error_response(self, err, request) -> ErrorResponse:
+ try:
err_body = err.read()
+ except (IncompleteRead, OSError):
+ raise self._http_response_exception(
+ err, request, "Unable to read error response"
+ ) from None
- if err_body is None or len(err_body) == 0:
- return (
- False,
- ErrorResponse.generate_error_response(RESTException,
err.reason),
- )
+ if not err_body:
+ raise self._http_response_exception(
+ err, request, "Empty error response"
+ ) from None
- err_resp = self._parse_error_response(err_body)
- err_resp.validate()
+ try:
+ response = self._parse_error_response(err_body)
+ response.validate()
+ except (ValueError, TypeError, KeyError, BadRequestException):
Review Comment:
Added `RecursionError` to the parse/validation catch, retaining the
fixed-reason `HTTPResponseException` raised `from None`. The nested-body
regression checks HTTP context, response closure and that the private HTTP
reason and underlying chain are absent from the formatted traceback. A custom
handler's `RecursionError` is also verified to propagate unchanged. The
complete unit suite and real HTTP regression module pass.
--
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]
