cshuo commented on code in PR #19659:
URL: https://github.com/apache/hudi/pull/19659#discussion_r3809449185
##########
hudi-io/src/main/java/org/apache/hudi/storage/HoodieStorage.java:
##########
@@ -329,56 +329,55 @@ public final void createImmutableFileInPath(StoragePath
path,
public final void createImmutableFileInPath(StoragePath path,
Option<HoodieInstantWriter>
contentWriter,
boolean needTempFile) throws
HoodieIOException {
- OutputStream fsout = null;
StoragePath tmpPath = null;
+ StoragePath pathToCreate = path;
+ if (contentWriter.isPresent() && needTempFile) {
+ StoragePath parent = path.getParent();
+ tmpPath = new StoragePath(parent, path.getName() + "." +
UUID.randomUUID());
+ pathToCreate = tmpPath;
+ }
- try {
- if (!contentWriter.isPresent()) {
- fsout = create(path, false);
- }
-
- if (contentWriter.isPresent() && needTempFile) {
- StoragePath parent = path.getParent();
- tmpPath = new StoragePath(parent, path.getName() + "." +
UUID.randomUUID());
- fsout = create(tmpPath, false);
- contentWriter.get().writeToStream(fsout);
- }
-
- if (contentWriter.isPresent() && !needTempFile) {
- fsout = create(path, false);
+ boolean writeSucceeded = false;
+ try (OutputStream fsout = create(pathToCreate, false)) {
+ if (contentWriter.isPresent()) {
contentWriter.get().writeToStream(fsout);
}
+ writeSucceeded = true;
} catch (IOException e) {
- String errorMsg = "Failed to create file " + (tmpPath != null ? tmpPath
: path);
- throw new HoodieIOException(errorMsg, e);
- } finally {
+ String errorMsg = (writeSucceeded ? "Failed to close file " : "Failed to
create file ") + pathToCreate;
+ HoodieIOException failure = new HoodieIOException(errorMsg, e);
+ deleteTemporaryFile(tmpPath, failure);
+ throw failure;
+ }
+
+ if (tmpPath != null) {
try {
- if (null != fsout) {
- fsout.close();
+ if (!rename(tmpPath, path)) {
+ deleteTemporaryFile(tmpPath, null);
+ LOG.debug("Failed to rename {} to {}; target file may already
exist", tmpPath, path);
}
} catch (IOException e) {
- String errorMsg = "Failed to close file " + (needTempFile ? tmpPath :
path);
- throw new HoodieIOException(errorMsg, e);
+ HoodieIOException failure = new HoodieIOException(
+ "Failed to rename " + tmpPath + " to the target " + path, e);
+ deleteTemporaryFile(tmpPath, failure);
+ throw failure;
}
+ }
+ }
- boolean renameSuccess = false;
- try {
- if (null != tmpPath) {
- renameSuccess = rename(tmpPath, path);
- }
- } catch (IOException e) {
- throw new HoodieIOException(
- "Failed to rename " + tmpPath + " to the target " + path,
- e);
- } finally {
- if (!renameSuccess && null != tmpPath) {
- try {
- deleteFile(tmpPath);
- LOG.debug("Failed to rename {} to {}, target file exists: {}",
tmpPath, path, exists(path));
- } catch (IOException e) {
- throw new HoodieIOException("Failed to delete tmp file " +
tmpPath, e);
- }
- }
+ private void deleteTemporaryFile(StoragePath tmpPath, HoodieIOException
primaryFailure) {
+ if (tmpPath == null) {
+ return;
+ }
+
+ try {
+ deleteFile(tmpPath);
+ } catch (IOException e) {
Review Comment:
The cleanup path only catches `IOException`, but the storage implementations
can also throw unchecked Hudi exceptions. For example,
`HoodieHadoopStorage.deleteFile()` throws `HoodieIOException` when deletion
returns false while the file still exists, and
`HoodieWrapperFileSystem.rename()` can throw `HoodieException` on a
consistency-guard timeout.
Consequently, a runtime rename failure now bypasses temporary-file
cleanup—whereas the previous `finally` block still ran—and a runtime cleanup
failure replaces the original write/close failure instead of being attached as
suppressed. Please handle these unchecked storage failures as well and add
coverage for rename and cleanup exceptions.
--
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]
