danny0405 commented on code in PR #19659:
URL: https://github.com/apache/hudi/pull/19659#discussion_r3809976086
##########
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:
Thanks for catching this. Updated in 7d7957ba018a:
- Create/write/close/rename failures are handled through a single `catch
(Throwable)`, and temporary-file cleanup runs from `finally` whenever the final
file was not created.
- Cleanup also catches `Throwable`; when there is a primary failure, a
cleanup failure is attached with `addSuppressed` instead of replacing it.
- Added regression coverage for an unchecked rename failure and an unchecked
cleanup failure.
The focused `TestHoodieHadoopStorage` suite passes all 15 tests, and
checkstyle reports 0 violations.
--
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]
