gaborkaszab commented on code in PR #16149:
URL: https://github.com/apache/iceberg/pull/16149#discussion_r3161971151
##########
core/src/main/java/org/apache/iceberg/rest/HTTPClient.java:
##########
@@ -317,54 +314,61 @@ protected <T extends RESTResponse> T execute(
request.setEntity(new StringEntity(encodedBody));
}
- HttpContext context = HttpClientContext.create();
- try (CloseableHttpResponse response = httpClient.execute(request,
context)) {
- 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 (emptyBody(response, responseType)) {
- if (response.getCode() == HttpStatus.SC_NOT_MODIFIED
- && !req.headers().contains(HttpHeaders.IF_NONE_MATCH)) {
- // 304-NOT_MODIFIED is used for freshness-aware loading and requires
an ETag sent to the
- // server via IF_NONE_MATCH header in the request. If no ETag was
sent, we shouldn't
- // receive a 304.
- throw new RESTException(
- "Invalid (NOT_MODIFIED) response for request: method=%s,
path=%s",
- req.method(), req.path());
- }
-
- return null;
- }
-
- if (!isSuccessful(response)) {
- // The provided error handler is expected to throw, but a
RESTException is thrown if not.
- String responseBody = extractResponseBodyAsString(response);
- throwFailure(response, responseBody, errorHandler);
- }
-
- if (response.getEntity() == null) {
- throw new RESTException(
- "Invalid (null) response body for request (expected %s):
method=%s, path=%s, status=%d",
- responseType.getSimpleName(), req.method(), req.path(),
response.getCode());
- }
-
- ObjectReader reader = objectReaderCache.computeIfAbsent(responseType,
mapper::readerFor);
- if (parserContext != null && !parserContext.isEmpty()) {
- reader = reader.with(parserContext.toInjectableValues());
- }
- return reader.readValue(response.getEntity().getContent());
+ try {
+ return httpClient.execute(
Review Comment:
I see before we used the signature with HttpContext and now you went for the
one without. Just to be on the safe side, since we passed an "empty context"
this is fine here, right?
##########
core/src/main/java/org/apache/iceberg/rest/HTTPClient.java:
##########
@@ -317,54 +314,61 @@ protected <T extends RESTResponse> T execute(
request.setEntity(new StringEntity(encodedBody));
}
- HttpContext context = HttpClientContext.create();
- try (CloseableHttpResponse response = httpClient.execute(request,
context)) {
- 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 (emptyBody(response, responseType)) {
- if (response.getCode() == HttpStatus.SC_NOT_MODIFIED
- && !req.headers().contains(HttpHeaders.IF_NONE_MATCH)) {
- // 304-NOT_MODIFIED is used for freshness-aware loading and requires
an ETag sent to the
- // server via IF_NONE_MATCH header in the request. If no ETag was
sent, we shouldn't
- // receive a 304.
- throw new RESTException(
- "Invalid (NOT_MODIFIED) response for request: method=%s,
path=%s",
- req.method(), req.path());
- }
-
- return null;
- }
-
- if (!isSuccessful(response)) {
- // The provided error handler is expected to throw, but a
RESTException is thrown if not.
- String responseBody = extractResponseBodyAsString(response);
- throwFailure(response, responseBody, errorHandler);
- }
-
- if (response.getEntity() == null) {
- throw new RESTException(
- "Invalid (null) response body for request (expected %s):
method=%s, path=%s, status=%d",
- responseType.getSimpleName(), req.method(), req.path(),
response.getCode());
- }
-
- ObjectReader reader = objectReaderCache.computeIfAbsent(responseType,
mapper::readerFor);
- if (parserContext != null && !parserContext.isEmpty()) {
- reader = reader.with(parserContext.toInjectableValues());
- }
- return reader.readValue(response.getEntity().getContent());
+ try {
Review Comment:
nit: maybe introducing a separate function for the response handler
implementation instead of inline lambda? Might help with the indentations, and
could introduce some flexibility for testing too later on.
--
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]
