adnanhemani commented on code in PR #9385:
URL: https://github.com/apache/iceberg/pull/9385#discussion_r1437959281
##########
aws/src/main/java/org/apache/iceberg/aws/s3/S3FileIOProperties.java:
##########
@@ -749,4 +795,23 @@ public <T extends S3ClientBuilder> void
applyEndpointConfigurations(T builder) {
builder.endpointOverride(URI.create(endpoint));
}
}
+
+ /**
+ * Add the S3 Access Grants Plugin for an S3 client.
+ *
+ * <p>Sample usage:
+ *
+ * <pre>
+ *
S3Client.builder().applyMutation(s3FileIOProperties::applyS3AccessGrantsConfigurations)
+ * </pre>
+ */
+ public <T extends S3ClientBuilder> void applyS3AccessGrantsConfigurations(T
builder) {
+ if (isS3AccessGrantsEnabled) {
+ S3AccessGrantsPlugin s3AccessGrantsPlugin =
Review Comment:
While looking into this, I don't think even the
ApacheHttpClientConfigurations reflection workaround that was implemented
earlier truly avoids a compile time dependency to be fair. I agree it may allow
us to not need to load both conflicting JARs in at runtime, but while compiling
the iceberg-aws module, you'll still need both ApacheHttpClient and
UrlConnectionHttpClient classes.
As a result, I'm interpreting what you're saying as: we should allow users
to choose to add this dependency or not at runtime itself. But I don't think
the same issues we were trying to solve in the
ApacheHttpClient/UrlConnectionHttpClient case are (or ever will be) applicable
here. Any number of AWS SDK Plugins can be attached on an AWS Client - and any
collision in behavior between them are solely the responsibility of the
customer defining the right set of plugins to add to the client (in other
words, the mere presence of multiple plugins in the classpath cannot cause
issues - only the runtime adding of the plugins to the client, which we cannot
control anyways).
I think this leaves one last concern (in my opinion): increased file size of
the iceberg-aws module JAR. When I try to exclude the unnecessary dependencies
from the S3 Access Grants plugin jar, it only increases the iceberg-aws module
JAR's size from ~29MB to ~31MB (an increase of ~2MB). That change is not
reflected in the PR, but if we are okay with including the plugin within the
iceberg-aws module jar, I can make that change to the PR quickly.
So, I think the question really boils down to: is the 2MB significant enough
that it is problematic to package out of the box with Iceberg? If you think so,
I can invest the time to add the reflection logic in. If not, I don't really
see a reason to make such a heavy change (adding reflection and classes) here.
--
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
