nastra commented on code in PR #12566:
URL: https://github.com/apache/iceberg/pull/12566#discussion_r2011668723
##########
aws/src/main/java/org/apache/iceberg/aws/s3/signer/S3V4RestSignerClient.java:
##########
@@ -250,25 +251,28 @@ public SdkHttpFullRequest sign(
} else {
Map<String, String> responseHeaders = Maps.newHashMap();
Consumer<Map<String, String>> responseHeadersConsumer =
responseHeaders::putAll;
- S3SignResponse s3SignResponse =
- httpClient()
- .withAuthSession(authSession())
- .post(
- endpoint(),
- remoteSigningRequest,
- S3SignResponse.class,
- Map.of(),
- ErrorHandlers.defaultErrorHandler(),
- responseHeadersConsumer);
-
- signedComponent =
- ImmutableSignedComponent.builder()
- .headers(s3SignResponse.headers())
- .signedURI(s3SignResponse.uri())
- .build();
-
- if (canBeCached(responseHeaders)) {
- SIGNED_COMPONENT_CACHE.put(cacheKey, signedComponent);
+ try (RESTClient restClient =
httpClient().withAuthSession(authSession())) {
Review Comment:
given that we're not closing anything I would not wrap this in a
try-with-resources here, because I had to go over this code a few times to make
sure we're not causing a performance degradation, since this part will be
called quite often during signing. Other readers of the code might have the
same thought and arrive at the same conclusion after studying the code and
that's why I think it's better to not use try-with-resources here (and most
likely in all of the places in the `RESTSessionCatalog`.
@danielcweeks would love to hear what you think about this?
--
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
