danny0405 commented on code in PR #18776:
URL: https://github.com/apache/hudi/pull/18776#discussion_r4061657382
##########
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:
Fixed in f0708d42dc51. `FileGroupReaderBasedMergeHandle.doMerge()` now
catches `Exception` in its existing reader/write block and calls the shared
`closeFileWriterQuietly` helper before throwing `HoodieUpsertException` with
the original cause. The helper closes and clears the writer and suppresses any
cleanup failure. `testFileGroupMergeWriteFailureClosesWriter` covers a failed
record write with ignore.failed=false, reader/iterator closure, and a failing
writer close. All 11 tests in `TestSortedAndChangeLogMergeHandles` pass.
##########
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:
Fixed in f0708d42dc51. If `super.close()` throws,
`HoodieMergeHandleWithChangeLog.close()` now closes `cdcLogger` with
`CloseableUtils.closeSuppressing` and rethrows the original exception. The
successful close path is unchanged. `testMergeCloseFailureClosesCDCWriter`
verifies both writers are closed and a CDC cleanup failure is suppressed onto
the original merge-close exception.
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/io/BaseCreateHandle.java:
##########
@@ -128,6 +129,7 @@ protected void doWrite(HoodieRecord record, HoodieSchema
schema, TypedProperties
} catch (Throwable t) {
log.error("Error writing record {}", record, t);
if (!config.getIgnoreWriteFailed()) {
+ closeFileWriterQuietly(t);
Review Comment:
I am keeping `FlinkCreateHandle.closeGracefully()` unchanged in this PR.
During task shutdown/client close, the failed-write path has already released
the writer. Leaving the finalized partial file for the existing marker/retry
cleanup is acceptable for this resource-release patch; finalization also leaves
the file readable for cleanup before commit. This PR does not add immediate
deletion for this case.
--
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]
