amogh-jahagirdar commented on code in PR #14161:
URL: https://github.com/apache/iceberg/pull/14161#discussion_r2446372776
##########
aws/src/main/java/org/apache/iceberg/aws/s3/PrefixedS3Client.java:
##########
@@ -50,12 +53,12 @@ class PrefixedS3Client implements AutoCloseable {
this.s3FileIOProperties = new S3FileIOProperties(properties);
// Do not override s3 client if it was provided
if (s3 == null) {
- Object clientFactory = S3FileIOAwsClientFactories.initialize(properties);
- if (clientFactory instanceof S3FileIOAwsClientFactory) {
- this.s3 = ((S3FileIOAwsClientFactory) clientFactory)::s3;
- }
- if (clientFactory instanceof AwsClientFactory) {
- this.s3 = ((AwsClientFactory) clientFactory)::s3;
+ Object factory = S3FileIOAwsClientFactories.initialize(properties);
+ if (factory instanceof S3FileIOAwsClientFactory) {
+ this.clientFactory = (S3FileIOAwsClientFactory) factory;
+ this.s3 = clientFactory::s3;
+ } else if (factory instanceof AwsClientFactory) {
+ this.s3 = ((AwsClientFactory) factory)::s3;
Review Comment:
Minor: Could we undo this naming change and refactoring, and the one right
below for async. It makes the diff larger than needed, and reviewers end up
having to analyze these lines to make sure things are still OK.
##########
aws/src/main/java/org/apache/iceberg/aws/s3/S3FileIOAwsClientFactory.java:
##########
@@ -44,4 +44,13 @@ public interface S3FileIOAwsClientFactory extends
Serializable {
* @param properties catalog properties
*/
void initialize(Map<String, String> properties);
+
+ /**
+ * Get the HTTP client key used for resource management.
+ *
+ * @return HTTP client key, or null if not available
+ */
+ default String httpClientKey() {
Review Comment:
It's worth thinking through if there's other reasonable ways to structure
this and store/supply the key state. If we had to do it similar to how it's
structured now, I think I'd still prefer some sort of
`SupportsHttpClientTracking` mixin which S3FileIOAwsClientFactory implements,
and then the AWsClientFacotry also extends this mixin.
That feels at least a little bit better than forcing AwsClientFactory to
extend S3FileIOAwsClientFactory which is backwards imo.
##########
aws/src/main/java/org/apache/iceberg/aws/s3/S3FileIOAwsClientFactory.java:
##########
@@ -44,4 +44,13 @@ public interface S3FileIOAwsClientFactory extends
Serializable {
* @param properties catalog properties
*/
void initialize(Map<String, String> properties);
+
+ /**
+ * Get the HTTP client key used for resource management.
+ *
+ * @return HTTP client key, or null if not available
+ */
+ default String httpClientKey() {
Review Comment:
I feel like it's a bit strange for S3FileIOClientFactory to have an
interface to get the httpClientKey. It puts the code in a bit of an awkward
spot where AwsClientFactory has to implement S3FileIO, which is an inversion of
the heirarchy. Could we just introduce a mixin of some sort
`SupportsHttpClientTracking` which has this clientKey API?
##########
aws/src/main/java/org/apache/iceberg/aws/s3/S3FileIOAwsClientFactory.java:
##########
@@ -44,4 +44,13 @@ public interface S3FileIOAwsClientFactory extends
Serializable {
* @param properties catalog properties
*/
void initialize(Map<String, String> properties);
+
+ /**
+ * Get the HTTP client key used for resource management.
+ *
+ * @return HTTP client key, or null if not available
+ */
+ default String httpClientKey() {
Review Comment:
cc @danielcweeks @nastra if they have any ideas here
##########
aws/src/main/java/org/apache/iceberg/aws/s3/S3FileIOAwsClientFactory.java:
##########
@@ -44,4 +44,13 @@ public interface S3FileIOAwsClientFactory extends
Serializable {
* @param properties catalog properties
*/
void initialize(Map<String, String> properties);
+
+ /**
+ * Get the HTTP client key used for resource management.
+ *
+ * @return HTTP client key, or null if not available
+ */
+ default String httpClientKey() {
Review Comment:
Ok I think I see why it was done this way. We need a way to capture the
client key , and there's no way to poke inside the underlying http client
(other than possibly some reflection hack), so we maintain this state in the
client factory.
--
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]
