voonhous commented on code in PR #18776:
URL: https://github.com/apache/hudi/pull/18776#discussion_r4060619198
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/io/HoodieWriteMergeHandle.java:
##########
@@ -506,10 +500,32 @@ public List<WriteStatus> close() {
return Collections.singletonList(writeStatus);
} catch (IOException e) {
+ closeFileWriterQuietly(e);
throw new HoodieUpsertException("Failed to close UpdateHandle", e);
+ } catch (RuntimeException e) {
+ closeFileWriterQuietly(e);
+ throw e;
+ } finally {
+ keyToNewRecords = null;
+ writtenRecordKeys = null;
}
}
+ private void closeFileWriter() throws IOException {
+ try {
+ if (fileWriter != null) {
+ fileWriter.close();
+ }
+ } finally {
+ fileWriter = null;
+ }
+ }
+
+ private void closeFileWriterQuietly(Throwable failure) {
Review Comment:
**major:** This cleanup only runs from `close()`, and the default merge
handle never gets there on a write failure. `FileGroupReaderBasedMergeHandle`
is the default for upsert and compaction (`HoodieWriteConfig:977`, `:992`), but
`MergeUtils.runMerge:120-128` and `HoodieCompactor:172-173` only call `close()`
after `doMerge()` succeeds, so with ignore.failed=false (Flink's default) a
failed record leaves `fileWriter` open. `HoodieMergeHelper:160-171` has the
same gap for this handle under the SIMPLE executor. Could `doMerge()` close the
writer quietly on failure, or `runMerge` / `compact` close the handle in a
`finally`? If that is out of scope, could the PR description say the default
merge handle is not covered?
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/io/HoodieWriteMergeHandle.java:
##########
@@ -506,10 +500,32 @@ public List<WriteStatus> close() {
return Collections.singletonList(writeStatus);
} catch (IOException e) {
+ closeFileWriterQuietly(e);
throw new HoodieUpsertException("Failed to close UpdateHandle", e);
+ } catch (RuntimeException e) {
+ closeFileWriterQuietly(e);
+ throw e;
Review Comment:
**minor:** When this rethrows, `HoodieMergeHandleWithChangeLog.close()`
(:114-125) skips `cdcLogger.close()`, since it calls `super.close()` with no
`finally`; the CDC log writer plus its `ExternalSpillableMap`
(`HoodieCDCLogger:239-250`) stay open. Not blocking, but could the override
close `cdcLogger` with `closeSuppressing` onto the `super.close()` failure?
--
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]
