gaborkaszab commented on code in PR #11870:
URL: https://github.com/apache/iceberg/pull/11870#discussion_r1908469683
##########
core/src/main/java/org/apache/iceberg/rest/HTTPClient.java:
##########
@@ -302,42 +302,48 @@ private <T> T execute(
addRequestHeaders(request, headers,
ContentType.APPLICATION_JSON.getMimeType());
}
- try (CloseableHttpResponse response = httpClient.execute(request)) {
- Map<String, String> respHeaders = Maps.newHashMap();
- for (Header header : response.getHeaders()) {
- respHeaders.put(header.getName(), header.getValue());
- }
-
- responseHeaders.accept(respHeaders);
-
- // Skip parsing the response stream for any successful request not
expecting a response body
- if (response.getCode() == HttpStatus.SC_NO_CONTENT
- || (responseType == null && isSuccessful(response))) {
- return null;
- }
-
- String responseBody = extractResponseBodyAsString(response);
-
- if (!isSuccessful(response)) {
- // The provided error handler is expected to throw, but a
RESTException is thrown if not.
- throwFailure(response, responseBody, errorHandler);
- }
-
- if (responseBody == null) {
- throw new RESTException(
- "Invalid (null) response body for request (expected %s):
method=%s, path=%s, status=%d",
- responseType.getSimpleName(), method.name(), path,
response.getCode());
- }
-
- try {
- return mapper.readValue(responseBody, responseType);
- } catch (JsonProcessingException e) {
- throw new RESTException(
- e,
- "Received a success response code of %d, but failed to parse
response body into %s",
- response.getCode(),
- responseType.getSimpleName());
- }
+ try {
Review Comment:
The other execute() function we called throws IOException while this one can
additionally throw ClientProtocolException. Not sure if we have to catch it
here or not, though, just wanted to raise attention.
##########
core/src/main/java/org/apache/iceberg/rest/HTTPClient.java:
##########
@@ -302,42 +302,48 @@ private <T> T execute(
addRequestHeaders(request, headers,
ContentType.APPLICATION_JSON.getMimeType());
}
- try (CloseableHttpResponse response = httpClient.execute(request)) {
- Map<String, String> respHeaders = Maps.newHashMap();
- for (Header header : response.getHeaders()) {
- respHeaders.put(header.getName(), header.getValue());
- }
-
- responseHeaders.accept(respHeaders);
-
- // Skip parsing the response stream for any successful request not
expecting a response body
- if (response.getCode() == HttpStatus.SC_NO_CONTENT
- || (responseType == null && isSuccessful(response))) {
- return null;
- }
-
- String responseBody = extractResponseBodyAsString(response);
-
- if (!isSuccessful(response)) {
- // The provided error handler is expected to throw, but a
RESTException is thrown if not.
- throwFailure(response, responseBody, errorHandler);
- }
-
- if (responseBody == null) {
- throw new RESTException(
- "Invalid (null) response body for request (expected %s):
method=%s, path=%s, status=%d",
- responseType.getSimpleName(), method.name(), path,
response.getCode());
- }
-
- try {
- return mapper.readValue(responseBody, responseType);
- } catch (JsonProcessingException e) {
- throw new RESTException(
- e,
- "Received a success response code of %d, but failed to parse
response body into %s",
- response.getCode(),
- responseType.getSimpleName());
- }
+ try {
+ return httpClient.execute(
+ request,
+ response -> {
Review Comment:
The change in general makes sense to me. Just a minor question: Do you think
it would make sense to move this logic into a ResponseHandler member of this
class? Seems a bit long to me to include it here as a lambda, and we'd save
some indentation too if it was a separate member.
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
