[HUDI-9336] Extract common logic of getting reader for secondary index #13212
Conversation
|
|
||
| Option<HoodieFileReader> baseFileReader = Option.empty(); | ||
| if (dataFilePath.isPresent()) { | ||
| baseFileReader = Option.of(HoodieIOFactory.getIOFactory(metaClient.getStorage()).getReaderFactory(recordMerger.getRecordType()).getFileReader(getReaderConfigs(storageConf), dataFilePath.get())); |
There was a problem hiding this comment.
This reader is never getting closed and it will lead to leaks.
There was a problem hiding this comment.
Yes. This is fixed now.
| Option<StoragePath> dataFilePath, | ||
| HoodieIndexDefinition indexDefinition, | ||
| String instantTime) throws Exception { | ||
| HoodieFileSliceReader fileSliceReader = |
There was a problem hiding this comment.
+1 for closing it after the iterator is consumed after while loop.
yihua
force-pushed
the
HUDI-9336-extract-reader-logic
branch
from
273427f to
6d3e45b
Compare
github-actions
bot
added
size:M
| import java.util.Map; | ||
| import java.util.Properties; | ||
|
|
||
| public class HoodieFileSliceReader<T> extends LogFileIterator<T> { |
There was a problem hiding this comment.
This class is going to be removed and all the usage will be replaced by HoodieFileGroupReader. So fixing the close behavior in a simple way in this PR.
|
|
||
| Option<HoodieFileReader> baseFileReader = Option.empty(); | ||
| if (dataFilePath.isPresent()) { | ||
| baseFileReader = Option.of(HoodieIOFactory.getIOFactory(metaClient.getStorage()).getReaderFactory(recordMerger.getRecordType()).getFileReader(getReaderConfigs(storageConf), dataFilePath.get())); |
There was a problem hiding this comment.
Yes. This is fixed now.
| metaClient.getTableConfig().getProps(), | ||
| Option.empty(), Option.empty()); | ||
| ClosableIterator<HoodieRecord> fileSliceIterator = ClosableIterator.wrap(fileSliceReader); | ||
| return new ClosableIterator<HoodieRecord>() { |
There was a problem hiding this comment.
This closable iterator and the file slice reader are going to be closed properly after #13178 is landed.
the-other-tim-brown
left a comment
the-other-tim-brown
left a comment
There was a problem hiding this comment.
Overall the refactor looks good to me. I am assuming there are already tests covering these paths
Yes, existing tests cover the logic. |
4 participants
Change Logs
This PR extracts common logic of getting the file slice reader for secondary index to reduce code duplication. It also makes changes to close resources in the file slice reader and the iterator for the secondary index properly.
Impact
Improves code maintainability and resource management
Risk level
none
Documentation Update
none
Contributor's checklist