mattcasters commented on code in PR #8843:
URL: https://github.com/apache/hop/pull/8843#discussion_r4237442007
##########
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:
**[bug]** `connectionOptions` is resolved once and then reused for every
lookup. `buildConnectionOptions()` copies region, endpoint, keys, session
token, credentials file, profile, and cache TTL from the provider's
`IVariables` at that moment. That provider is the one registered for the VFS
manager (the GUI session, or a namespace) and is documented as what named
connections resolve credentials with. Editing a project variable, a
parent-scope `Set variables` value, or a `#{resolver:...}` secret does not call
`HopVfs.refresh()` — only saving the connection replaces the provider — so
later resolves keep the first snapshot. A rotated access key or session token
is ignored until then.
Holding one options instance is not what makes the cache hit. Commons VFS
2.10 stores file systems in a `TreeMap` keyed by `FileSystemKey.compareTo`, and
`FileSystemOptions.compareTo` treats equal contents as the same key. A newly
built options object with the same resolved values still returns the existing
`S3FileSystem`.
**Suggestion:** Build the named connection's options on each `getFileSystem`
/ `doCreateFileSystem` call instead of caching the instance. Unchanged settings
still reuse one file system and one S3 client. When the resolved values differ,
close and drop the previous file system before the new one is cached so old
clients do not accumulate. Extend `S3FileProviderFileSystemReuseTest` with a
variable change between two resolves.
--
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]
