hansva commented on code in PR #8843:
URL: https://github.com/apache/hop/pull/8843#discussion_r4237882097
##########
plugins/tech/aws/src/main/java/org/apache/hop/vfs/s3/s3/vfs/S3FileProvider.java:
##########
@@ -53,80 +57,101 @@ public S3FileProvider(IVariables variables, S3Meta s3Meta)
{
setFileNameParser(S3FileNameParser.getInstance());
}
+ /**
+ * Look file systems up by the options they are created with. Commons VFS
caches a file system
+ * under its own options but looks it up under the caller's, which are empty
when resolving
+ * through HopVfs; a named connection's options never matched, so every
resolve built a new file
+ * system and S3 client, and the provider kept all of them.
+ */
+ @Override
+ protected synchronized FileSystem getFileSystem(
+ final FileName rootFileName, final FileSystemOptions fileSystemOptions)
+ throws FileSystemException {
+ return super.getFileSystem(rootFileName, options(fileSystemOptions));
+ }
+
@Override
public FileSystem doCreateFileSystem(
final FileName name, final FileSystemOptions fileSystemOptions) {
- // Named connections (s3test:// etc.) must use a dedicated options
instance so we never
- // mutate the default options used by s3://. Otherwise using s3test://
would overwrite
- // the default S3 configuration.
- final FileSystemOptions options;
- if (s3Meta != null && variables != null) {
- options = new FileSystemOptions();
- } else {
- options = fileSystemOptions != null ? fileSystemOptions :
getDefaultFileSystemOptions();
+ return new S3FileSystem(name, options(fileSystemOptions));
+ }
+
+ /**
+ * The options of the file system for the given request. For s3:// the
caller's options, or the
+ * defaults. For a named connection one options instance, built from the
connection the first time
+ * it is needed: dedicated, so it never changes the defaults s3:// uses, and
the same every time,
+ * so its file system is found again.
+ */
+ private synchronized FileSystemOptions options(final FileSystemOptions
fileSystemOptions) {
+ if (s3Meta == null || variables == null) {
+ return fileSystemOptions != null ? fileSystemOptions :
getDefaultFileSystemOptions();
}
+ if (connectionOptions == null) {
+ connectionOptions = buildConnectionOptions();
Review Comment:
can we defer this change or mark it as a feature request? On the snapshot:
that matches the other named providers. GCS resolves its credentials in the
provider constructor. Azure and MinIO resolve on first file system creation and
keep that file system for the provider's lifetime. All of them pick up changes
through HopVfs.refresh() when the connection is saved. Before this PR, S3 only
re-read variables because every resolve built a new file system and client,
which is the leak being fixed. Re-resolving on every lookup would also run
secret resolvers on every file resolve. Picking up variable or secret changes
without a save would be good, but it should be designed for all named providers
at once (refresh on change, or a TTL) rather than only S3. I'd keep that out of
this PR.
--
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]
