wombatu-kun commented on issue #8548:
URL: https://github.com/apache/paimon/issues/8548#issuecomment-5139672841
I looked into this and I am preparing a PR. Answering the two design
questions first, since the fix hinges on them.
**Is `HadoopFileIO` intended to own the `FileSystem` instances in its
`fsMap`?** Only some of them, and there is an exact test for which.
`createFileSystem` calls `path.getFileSystem(conf)`, which is Hadoop's
`FileSystem.get(URI, Configuration)`:
```java
String disableCacheName = String.format("fs.%s.impl.disable.cache", scheme);
if (conf.getBoolean(disableCacheName, false)) {
return createFileSystem(uri, conf); // a fresh instance nobody else
can reach
}
return CACHE.get(uri, conf); // shared, owned by Hadoop's
global cache
```
So a `HadoopFileIO` owns a `FileSystem` if and only if
`fs.<scheme>.impl.disable.cache` is true for the scheme of the path it was
created for. That is exactly the configuration you are running, which is why
you see one `AliyunOSSFileSystem` per `HadoopFileIO`.
**How should cleanup behave when the global cache is enabled?** It should do
nothing at all. Those instances are shared with every other user in the JVM,
including other `FileIO`s and the compute engine itself, and Hadoop releases
them through `FileSystem.closeAll()` on its shutdown hook. Closing one from
`HadoopFileIO.close()` would surface as `IOException: Filesystem closed` in
unrelated readers.
Two details that matter for getting the predicate right, both verified
against the Hadoop bytecode rather than assumed: the scheme comes from the
**path**, not from `FileSystem#getUri()`, and it is matched **as written** with
no lower casing, because that is what Hadoop itself looks up. Lower casing
would be the dangerous direction: a path spelled `OSS://bucket` is served from
the shared cache, but a case-insensitive check would find
`fs.oss.impl.disable.cache=true` and close it.
Beyond `HadoopFileIO.close()` itself, three things turned out to be needed
before any of it is reachable, so the PR covers them too:
- `ResolvingFileIO` and `PluginFileIO` cache delegates and never forwarded
`close()`, so the call was swallowed one level above `HadoopFileIO`. Notably
this also means `OSSFileIO.close()`, which already releases uncached file
systems, has never actually run in production.
- `HadoopSecuredFileSystem` extends `FileSystem` without overriding
`close()`, so on a Kerberised deployment the wrapped instance was never closed.
- `FileIO.checkAccess` probes a loader by calling `exists()` on a throwaway
`FileIO` and then drops it. With the cache disabled that probe creates a fully
initialised `FileSystem`, with its connector threads, on **every**
`FileIO.get()` call, and nothing ever closes it. This one leaks without anybody
calling `close()` at all, so it may well be the larger half of what you are
seeing.
Out of scope, and I would rather state it than leave it implicit: the
per-module `HadoopCompliantFileIO` copies under `paimon-filesystems` obtain
their file systems from module-level static caches, so releasing those per
instance needs a separate design.
One behaviour change worth flagging for reviewers: `RESTTokenFileIO` keeps a
static cache of `FileIO`s whose removal listener already calls `closeQuietly`.
That call has been landing on the empty default `close()`; once `close()` does
real work, a size-based eviction can release a file system another table is
still reading through. `OSSFileIO` already carries the same exposure today, so
this is not a new class of hazard, but it is new reach.
I will link the PR here shortly.
--
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]
