gaborkaszab commented on code in PR #14035:
URL: https://github.com/apache/iceberg/pull/14035#discussion_r2426087084
##########
core/src/main/java/org/apache/iceberg/rest/HTTPClient.java:
##########
@@ -360,6 +360,13 @@ protected <T extends RESTResponse> T execute(
}
}
+ private <T extends RESTResponse> boolean emptyBody(
+ CloseableHttpResponse response, Class<T> responseType) {
+ return response.getCode() == HttpStatus.SC_NO_CONTENT
+ || response.getCode() == HttpStatus.SC_NOT_MODIFIED
Review Comment:
This one is required here because this one triggers returning early with.
Without this we'd run into an "Invalid (null) response body" RESTException.
However, the additional check into `isSuccessful` is not necessary required
(same would be tru for SC_NO_CONTENT too), I think to be future proof it
semantically makes sense for an `isSuccessful` function to return true for 304
too even though the current code returns before calling that function. WDYT?
Additionally, when I experimented around this area I noticed that the
current test injects RESTCatalogAdapter directly into RESTSessionCatalog
avoiding this code path through HTTPClient. Let me rethink the test a little
bit so that we could go through the whole path.
##########
core/src/test/java/org/apache/iceberg/rest/RESTCatalogAdapter.java:
##########
@@ -433,8 +285,19 @@ public <T extends RESTResponse> T handleRequest(
tableIdentFromPathVars(vars),
snapshotModeFromQueryParams(httpRequest.queryParameters()));
- responseHeaders.accept(
- ImmutableMap.of(HttpHeaders.ETAG,
ETagProvider.of(response.metadataLocation())));
+ Optional<HTTPHeaders.HTTPHeader> ifNoneMatchHeader =
+ httpRequest.headers().entries().stream()
+ .filter(e -> e.name().equals(HttpHeaders.IF_NONE_MATCH))
+ .findFirst();
Review Comment:
Some of the classes from external hc.http source has such a function,
however, this is the internal, Iceberg specific `HTTPRequest` class that
doesn't. I can add such a function there, let me know if it makes sense (this
or separate PR)
##########
core/src/test/java/org/apache/iceberg/rest/RESTCatalogAdapter.java:
##########
@@ -114,147 +107,6 @@ public RESTCatalogAdapter(Catalog catalog) {
this.asViewCatalog = catalog instanceof ViewCatalog ? (ViewCatalog)
catalog : null;
}
- enum Route {
Review Comment:
Sure. Here it is: https://github.com/apache/iceberg/pull/14313
##########
core/src/test/java/org/apache/iceberg/rest/RESTCatalogAdapter.java:
##########
@@ -433,8 +285,19 @@ public <T extends RESTResponse> T handleRequest(
tableIdentFromPathVars(vars),
snapshotModeFromQueryParams(httpRequest.queryParameters()));
- responseHeaders.accept(
- ImmutableMap.of(HttpHeaders.ETAG,
ETagProvider.of(response.metadataLocation())));
+ Optional<HTTPHeaders.HTTPHeader> ifNoneMatchHeader =
+ httpRequest.headers().entries().stream()
+ .filter(e -> e.name().equals(HttpHeaders.IF_NONE_MATCH))
+ .findFirst();
+
+ String eTag = ETagProvider.of(response.metadataLocation());
+ Preconditions.checkNotNull(eTag, "ETag is null");
Review Comment:
Indeed, removed
--
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]
