Skip to content

[common] Forward close through PluginFileIO and ResolvingFileIO - #9472

Closed
wombatu-kun wants to merge 1 commit into
apache:masterfrom
wombatu-kun:issue/8548-forward-close-through-wrapping-fileios
Closed

[common] Forward close through PluginFileIO and ResolvingFileIO#9472
wombatu-kun wants to merge 1 commit into
apache:masterfrom
wombatu-kun:issue/8548-forward-close-through-wrapping-fileios

Conversation

@wombatu-kun

Copy link
Copy Markdown
Contributor

Purpose

Split out of #8962 at @JingsongLi's request. Answers this review comment.

PluginFileIO and ResolvingFileIO never forwarded close() to their delegates, unlike CachingFileIO, so OSSFileIO.close() has never run and RESTCatalog's try (FileIO fileIO = fileIOFromOptions(path)) releases nothing. Both forward now, and close() is terminal so the wrappers cannot silently rebuild a delegate nobody will release.

That also makes RESTTokenFileIO's removal listener close a delegate for real. Its cache is JVM wide and hands out raw FileIOs and streams, so an eviction could close an OSS file system while another table is still reading through it. Values are reference counted now: the listener hands back only the cache's own reference, and the delegate is closed when the last lease goes. Leases cover each operation and the lifetime of every stream and listing handed out, and the caller's lease is taken before the put, since the admission policy can evict a just-inserted entry.

BaseMultiPartUploadCommitter, LanceUtils and VortexUtils work inside a lease instead of unwrapping the cached instance. fileIO() is deprecated, since a raw reference carries no lifetime to track, but kept because engines cast the result to their own implementation.

Cut from current master and shares no file with #8962 or #9471, so the three can be reviewed in any order.

Tests

RESTTokenFileIOTest (new): an eviction keeps a leased FileIO alive and closes an unleased one; an open input, output or two-phase stream keeps its delegate alive until the stream ends; the wrapper carries VectoredReadable through and does not fake it; a failed open hands the lease back; a spent entry is rebuilt; a lease closed twice releases once; the multipart committer holds a lease across the call. PluginFileIOTest, ResolvingFileIOTest: close releases every delegate under the plugin classloader, keeps going when one fails, and use after close is rejected.

@Override
public SeekableInputStream newInputStream(Path path) throws IOException {
return fileIO().newInputStream(path);
Lease lease = acquire();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why need to modify RESTTokenFileIO?

@JingsongLi

Copy link
Copy Markdown
Contributor

I think this PR can be simplified and kept focused:

  • Keep the close forwarding in PluginFileIO and ResolvingFileIO.
  • In RESTTokenFileIO, stop closing cache values from the removal listener for now, instead of adding the lease/reference-counting API and changing the multipart, Lance, and Vortex callers in this PR.
  • Handle safe cleanup of REST-token cache entries in a follow-up after lazy listings have an explicit close contract.

Forwarding close() does make the current removal listener unsafe: cache eviction can close a delegate while a returned stream or iterator is still active. However, the lease implementation in this PR still cannot make that lifecycle correct. LeasedRemoteIterator releases only after exhaustion or an exception, while RemoteIterator is not closeable. A LIMIT, task cancellation, or any caller that abandons a listing early permanently leaks a reference, so the cached delegate can never be closed after token rotation or eviction.

Removing the listener preserves the previous effective behavior for the PluginFileIO / ResolvingFileIO paths, avoids closing a delegate while it is in use, and reduces this PR to the wrapper ownership fix. A follow-up can make RemoteIterator closeable with a default no-op for compatibility, propagate close through ObjectTable / IteratorRecordReader / FileIO.listFiles, and then re-enable reference-counted cache cleanup with an early-close regression test.

@JingsongLi

Copy link
Copy Markdown
Contributor

After reconsidering the scope and current priority, I do not think this PR is worth continuing at this point. The original issue has not been accepted for now, so the benefit does not seem large enough to justify introducing and maintaining this fairly broad lifecycle change, especially with the additional iterator and compatibility concerns discussed above.

I would prefer to stop here and close this PR rather than split it or expand the scope further. We can revisit the problem if the original issue is accepted and prioritized later, ideally with a smaller design.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants