Do not close Hadoop FileSystem instances

[electrum]

Description

Is this change a fix, improvement, new feature, refactoring, or other?

fix

Documentation

(x) No documentation is needed.

Release notes

(x) Release notes entries required with the following suggested text:

# Delta Lake connector
* Prevent potential failure of unrelated queries after dropping a schema. ({issue}`13810`)

# Hive connector
* Prevent potential failure of unrelated queries after dropping a schema. ({issue}`13810`)

[erichwang]

Do we want to change the SyncingFileSystem usage? This actually seemed fine before. For example, if you disable this, then you break the deleteOnExit behavior if that is being used.

[electrum]

It wasn't necessary to close before, as we didn't use that behavior. It seems better to simply remove the close than to suppress the warning.

[findepi]

"singletons" or "shared"?

[martint]

Please add this to the commit message so that the motivation for this change is clear

[electrum]

Not sure I understand the question. They are singletons and thus are shared.

[electrum]

Would it be more clear if I wrote "shared singletons"?

[findepi]

I don't like plural "singletons". To me a singletons is a one object.
Pool of shared objects is "shared objects" to me.

Also, FS instances have some lifecycle, right? They are created, cached and closed behind the scenes, am i correct?
So the application can eventually create unlimited number of these objects over time.

[martint]

There's no contradiction in the plural form of "singletons". Each type of FileSystem can be a singleton (one for S3, one for HDFS, one for Minio, etc), and you can still refer to them collectively as a group of singletons.

[findepi]

So we create as many FileSystem objects as many different URI schemes are in use?
Does the answer depend on extra credentials like for GCS?

[electrum]

I think it's best to say that "it's complicated" and you can find the exact logic is in TrinoFileSystemCache. I changed the wording to "shared" since it seems to be a better way to communicate why we shouldn't close them.

[findepi]

Will this be caught when the code uses explicit type, eg. SyncingFileSystem?
i'd guess the code might be compiled with actual implementation method name.

Maybe the shared FileSystem instances should be wrapped with something that prevents inadvertent close?

[electrum]

Yes, as long as it's not cast to Closeable. We're getting rid of FileSystem usage so I'd rather not make an invasive change like that.

[erichwang]

looks good to me. this needs to go in