You are viewing a plain text version of this content. The canonical link for it is here.

Posted to issues@paimon.apache.org by "wombatu-kun (via GitHub)" <gi...@apache.org> on 2026/08/30 03:50:39 UTC

[PR] [common] Forward close through PluginFileIO and ResolvingFileIO [paimon]

wombatu-kun opened a new pull request, #9472:
URL: https://github.com/apache/paimon/pull/9472

### Purpose

Split out of #8962 at @JingsongLi's request. Answers [this review comment](https://github.com/apache/paimon/pull/8962#discussion_r3703449620).

`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 `FileIO`s 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.

--
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: issues-unsubscribe@paimon.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [common] Forward close through PluginFileIO and ResolvingFileIO [paimon]

Posted by "wombatu-kun (via GitHub)" <gi...@apache.org>.

wombatu-kun closed pull request #9472: [common] Forward close through PluginFileIO and ResolvingFileIO
URL: https://github.com/apache/paimon/pull/9472


-- 
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: issues-unsubscribe@paimon.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [common] Forward close through PluginFileIO and ResolvingFileIO [paimon]

Posted by "JingsongLi (via GitHub)" <gi...@apache.org>.

JingsongLi commented on PR #9472:
URL: https://github.com/apache/paimon/pull/9472#issuecomment-5467439462

   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.


-- 
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: issues-unsubscribe@paimon.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [common] Forward close through PluginFileIO and ResolvingFileIO [paimon]

Posted by "JingsongLi (via GitHub)" <gi...@apache.org>.

JingsongLi commented on code in PR #9472:
URL: https://github.com/apache/paimon/pull/9472#discussion_r3888735079


##########
paimon-common/src/main/java/org/apache/paimon/rest/RESTTokenFileIO.java:
##########
@@ -109,62 +119,126 @@ public void configure(CatalogContext context) {
 
     @Override
     public SeekableInputStream newInputStream(Path path) throws IOException {
-        return fileIO().newInputStream(path);
+        Lease lease = acquire();

Review Comment:
   Why need to modify RESTTokenFileIO?



-- 
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: issues-unsubscribe@paimon.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [common] Forward close through PluginFileIO and ResolvingFileIO [paimon]

Posted by "JingsongLi (via GitHub)" <gi...@apache.org>.

JingsongLi commented on PR #9472:
URL: https://github.com/apache/paimon/pull/9472#issuecomment-5467433288

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.

--
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: issues-unsubscribe@paimon.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org