jackye1995 commented on a change in pull request #4334:
URL: https://github.com/apache/iceberg/pull/4334#discussion_r828411959
##########
File path: aws/src/main/java/org/apache/iceberg/aws/s3/S3FileIO.java
##########
@@ -101,17 +102,17 @@ public S3FileIO(SerializableSupplier<S3Client> s3,
AwsProperties awsProperties)
@Override
public InputFile newInputFile(String path) {
- return S3InputFile.fromLocation(path, client(), awsProperties, metrics);
+ return S3InputFile.fromLocation(path, client(), awsProperties, metrics,
bucketToAccessPointMapping);
}
@Override
public OutputFile newOutputFile(String path) {
- return S3OutputFile.fromLocation(path, client(), awsProperties, metrics,
writeTags);
+ return S3OutputFile.fromLocation(path, client(), awsProperties, metrics,
writeTags, bucketToAccessPointMapping);
Review comment:
I am thinking about this a bit, it does not really scale if we keep
adding new arguments to the constructor, we will basically have 1 new factory
method per release. Traditionally we have been adding all the properties
directly in `awsProperties`, and we did not really evaluate that option when
adding `writeTags`. We should think about this a bit, I feel it is cleaner if
we move all these new fields back to `AwsProperties`, it was designed to be the
extension point for new configs without changing constructors in different
places? Any thoughts?
--
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]
