LuciferYang opened a new issue, #9635:
URL: https://github.com/apache/paimon/issues/9635

   ### Search before asking
   
   - [x] I searched in the [issues](https://github.com/apache/paimon/issues) 
and found nothing similar.
   
   ### Paimon version
   
   master, `475be566f` (2.1-SNAPSHOT).
   
   ### Compute Engine
   
   Any. Both remaining call sites are in paimon-core: `rewrite-file-index` and 
compaction go through `FileIndexProcessor`, and the `$file_indexes` system 
table through `FileIndexesTable`.
   
   ### Minimal reproduce step
   
   Read a `.index` file whose magic or version does not match, for example a 
truncated or replaced index file, through either of these:
   
   ```java
   try (FileIndexFormat.Reader indexReader =
           FileIndexFormat.createReader(
                   
fileIO.newInputStream(dataFilePathFactory.toAlignedPath(indexFile, 
dataFileMeta)),
                   schemaInfo.fileSchema)) {
   ```
   
   `FileIndexFormat.Reader`'s constructor validates the header and throws 
`RuntimeException("This file is not file index file.")`. Its `catch` only 
covers `IOException`:
   
   ```java
   } catch (IOException e) {
       IOUtils.closeQuietly(seekableInputStream);
       throw new RuntimeException("Exception happens while construct file index 
reader.", e);
   }
   ```
   
   A constructor that throws never assigns the try-with-resources variable, so 
the stream `newInputStream(...)` just opened is never closed. On HDFS or an 
object store that is a leaked connection per failed read, and 
`FileIndexProcessor` runs per data file during compaction.
   
   `FileIndexPredicate` already works around this on its own since #9462, which 
added a `catch (RuntimeException)` at that call site. The other two call sites 
did not get the same treatment: `FileIndexProcessor` (line 98) and 
`FileIndexesTable` (line 366, and line 328 with an in-memory stream where it 
does not matter).
   
   ### What doesn't meet your expectations?
   
   The workaround belongs in the constructor rather than in each caller. The 
constructor is the only code holding a reference to the stream at that moment, 
so it is the only place that can release it, and #9462 had to add the same 
three lines that the next caller will have to add again.
   
   ### Anything else?
   
   Making the constructor close on `RuntimeException` also changes what callers 
see for a bad header: the bare `RuntimeException("This file is not file index 
file.")` becomes that same exception wrapped in `RuntimeException("Exception 
happens while construct file index reader.")`. 
`FileIndexesTable.fileIndexReadException` checks `getCause() instanceof 
IOException`, so it is unaffected.
   
   ### Are you willing to submit a PR?
   
   - [x] I'm willing to submit a PR!
   


-- 
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]

Reply via email to