gianm commented on code in PR #19317:
URL: https://github.com/apache/druid/pull/19317#discussion_r3270382057
##########
extensions-core/s3-extensions/src/main/java/org/apache/druid/storage/s3/output/S3ExportStorageProvider.java:
##########
@@ -93,7 +124,22 @@ public StorageConnector createStorageConnector(File
taskTempDir)
s3ExportConfig.getChunkSize(),
s3ExportConfig.getMaxRetry()
);
- return new S3StorageConnector(s3OutputConfig, s3, s3UploadManager);
+ if (Strings.isNullOrEmpty(assumeRoleArn)) {
+ return new S3StorageConnector(s3OutputConfig, s3Client, s3UploadManager);
+ }
+ return new S3StorageConnector(
+ s3OutputConfig,
+ ServerSideEncryptingAmazonS3.builder(
Review Comment:
Frank's automated review makes a good point about this here:
https://github.com/apache/druid/pull/19317/files#r3161338202
Each of these `ServerSideEncryptingAmazonS3` is going to leak some
resources: thread pools, connection pools, and the like. This was a
pre-existing problem with `S3InputSource` when a `S3InputDataConfig` is
provided (via the `properties` field). I think because we have an
`S3InputSource` per file (due to splitting), we'd potentially even in `master`
be creating a lot of `ServerSideEncryptingAmazonS3` when `properties` is set.
This PR isn't making it a ton worse, but it also isn't making it much
better. I suppose it would be OK to merge it given that it's not a ton worse,
but, could you please memoize the client in the constructor of
`S3ExportStorageProvider` (similar to what `S3InputSource` does). That will at
least allow it to be shared across calls to `createStorageConnector`.
Could you please also put some comments about the problem in
`ServerSideEncryptingAmazonS3#builder`. Ultimately fixing it would I think
involve making `ServerSideEncryptingAmazonS3` closeable, and arranging for
`close` to actually be called, which would be a larger change.
--
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]
