hudi-agent commented on code in PR #20030:
URL: https://github.com/apache/hudi/pull/20030#discussion_r4080216833
##########
hudi-common/src/main/java/org/apache/hudi/common/table/read/lsm/LsmFileGroupRecordIterator.java:
##########
Review Comment:
🤖 **Line 500:** Now that initialization cleanup closes every reader even
when one close fails, should the normal close path get the same treatment?
`leaves.forEach(SortedRunReader::close)` aborts on the first failure, so if one
reader's close throws (e.g. a spill iterator failing to delete its file), the
remaining readers and their spill files leak.
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-common/src/main/java/org/apache/hudi/common/table/read/lsm/LsmFileGroupRecordIterator.java:
##########
@@ -153,28 +154,37 @@ public LsmFileGroupRecordIterator(HoodieReaderContext<T>
readerContext,
*/
private List<SortedRunReader<T>> initializeReaders() throws IOException {
List<SortedRunReader<T>> sortedRunReaders = new ArrayList<>();
- int mergeOrder = 0;
- if (readBaseFile) {
- addReader(sortedRunReaders, mergeOrder++,
LsmFileIterators.createBaseFileIterator(
- readerContext, storage, inputSplit.getBaseFileOption().get(),
- inputSplit.getStart(), inputSplit.getLength(), orderingFieldNames,
false));
- }
+ try {
+ int mergeOrder = 0;
+ if (readBaseFile) {
+ addReader(sortedRunReaders, mergeOrder++,
LsmFileIterators.createBaseFileIterator(
+ readerContext, storage, inputSplit.getBaseFileOption().get(),
+ inputSplit.getStart(), inputSplit.getLength(), orderingFieldNames,
false));
+ }
- if (inputSplit.hasRecordIterator()) {
- addReader(sortedRunReaders, mergeOrder++,
createRecordIterator(inputSplit.getRecordIterator()));
- }
+ if (inputSplit.hasRecordIterator()) {
+ addReader(sortedRunReaders, mergeOrder++,
createRecordIterator(inputSplit.getRecordIterator()));
+ }
- List<LogReaderSpec> logReaderSpecs = new ArrayList<>();
- if (!inputSplit.hasRecordIterator()) {
- for (HoodieLogFile logFile : inputSplit.getLogFiles()) {
- logReaderSpecs.add(new LogReaderSpec(mergeOrder++, logFile));
+ List<LogReaderSpec> logReaderSpecs = new ArrayList<>();
+ if (!inputSplit.hasRecordIterator()) {
+ for (HoodieLogFile logFile : inputSplit.getLogFiles()) {
+ logReaderSpecs.add(new LogReaderSpec(mergeOrder++, logFile));
+ }
}
- }
- Set<Integer> directLogMergeOrders =
selectDirectLogMergeOrders(logReaderSpecs, readBaseFile);
- for (LogReaderSpec spec : logReaderSpecs) {
- ClosableIterator<BufferedRecord<T>> iterator =
LsmFileIterators.createLogFileIterator(
- readerContext, metaClient, storage, spec.logFile,
orderingFieldNames);
- addReader(sortedRunReaders, spec.mergeOrder,
maybeSpillIterator(directLogMergeOrders.contains(spec.mergeOrder), iterator));
+ Set<Integer> directLogMergeOrders =
selectDirectLogMergeOrders(logReaderSpecs, readBaseFile);
+ for (LogReaderSpec spec : logReaderSpecs) {
+ ClosableIterator<BufferedRecord<T>> iterator =
LsmFileIterators.createLogFileIterator(
+ readerContext, metaClient, storage, spec.logFile,
orderingFieldNames);
+ addReader(sortedRunReaders, spec.mergeOrder,
maybeSpillIterator(directLogMergeOrders.contains(spec.mergeOrder), iterator));
Review Comment:
🤖 nit: catching `Throwable` here is quite broad (it would also swallow
`Error`s like `OutOfMemoryError`) — could this be narrowed to `Exception`, or
is catching `Throwable` intentional to guarantee cleanup on any failure? Worth
a brief comment either way.
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
--
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]
