-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[HUDI-5353] Close file readers #7412
Conversation
try (HoodieFileReader<GenericRecord> bootstrapReader = | ||
HoodieFileReaderFactory.getFileReader(bootstrapFileConfig, bootstrapFilePath)) { | ||
return new MergingIterator<>(recordIterator, bootstrapReader.getRecordIterator(), | ||
(inputRecordPair) -> HoodieAvroUtils.stitchRecords(inputRecordPair.getLeft(), inputRecordPair.getRight(), mergeHandle.getWriterSchemaWithMetaFields())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the record iterator still valid if we close the reader already ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point! We should pull up the reader. I'll check other such places. Thanks.
try { | ||
return HoodieFileReaderFactory.getFileReader(table.getHadoopConf(), new Path(clusteringOp.getDataFilePath())).getRecordIterator(readerSchema); | ||
try (HoodieFileReader fileReader = HoodieFileReaderFactory.getFileReader(table.getHadoopConf(), new Path(clusteringOp.getDataFilePath()))) { | ||
return fileReader.getRecordIterator(readerSchema); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
* @param conf Configuration | ||
* @return | ||
*/ | ||
public static Schema getBaseFileSchema(FileSplit split, Configuration conf) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there are no usages for these is it ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes that's correct.
674fcb3
to
bf7a262
Compare
CI is green after retry. There is a flaky test unrelated to this PR and it's being tracked in https://issues.apache.org/jira/browse/HUDI-5371 |
Change Logs
Close file readers wherever missing.
Impact
Simply closes file reader. Better use of connection pool.
Risk level (write none, low medium or high below)
low
Documentation Update
Describe any necessary documentation update if there is any new feature, config, or user-facing change
ticket number here and follow the instruction to make
changes to the website.
Contributor's checklist