RussellSpitzer commented on code in PR #9546:
URL: https://github.com/apache/iceberg/pull/9546#discussion_r1505005414
##########
core/src/main/java/org/apache/iceberg/hadoop/HadoopTableOperations.java:
##########
@@ -368,58 +431,63 @@ private void renameToFinal(FileSystem fs, Path src, Path
dst, int nextVersion) {
if (fs.exists(dst)) {
throw new CommitFailedException("Version %d already exists: %s",
nextVersion, dst);
}
-
- if (!fs.rename(src, dst)) {
- CommitFailedException cfe =
- new CommitFailedException("Failed to commit changes using rename:
%s", dst);
- RuntimeException re = tryDelete(src);
- if (re != null) {
- cfe.addSuppressed(re);
- }
- throw cfe;
- }
- } catch (IOException e) {
- CommitFailedException cfe =
- new CommitFailedException(e, "Failed to commit changes using rename:
%s", dst);
- RuntimeException re = tryDelete(src);
- if (re != null) {
- cfe.addSuppressed(re);
+ if (!nextVersionIsLatest(nextVersion, fs)) {
+ throw new CommitFailedException("Version %d too old: %s", nextVersion,
dst);
}
- throw cfe;
+ return renameMetaDataFileAndCheck(fs, src, dst);
} finally {
if (!lockManager.release(dst.toString(), src.toString())) {
LOG.warn("Failed to release lock on file: {} with owner: {}", dst,
src);
}
}
}
- /**
- * Deletes the file from the file system. Any RuntimeException will be
caught and returned.
- *
- * @param path the file to be deleted.
- * @return RuntimeException caught, if any. null otherwise.
- */
- private RuntimeException tryDelete(Path path) {
- try {
- io().deleteFile(path.toString());
- return null;
- } catch (RuntimeException re) {
- return re;
- }
- }
-
protected FileSystem getFileSystem(Path path, Configuration hadoopConf) {
return Util.getFs(path, hadoopConf);
}
+ @VisibleForTesting
+ boolean checkMetaDataFileRenameSuccess(
+ FileSystem fs, Path tempMetaDataFile, Path finalMetaDataFile) throws
IOException {
+ return fs.exists(finalMetaDataFile) && !fs.exists(tempMetaDataFile);
+ }
+
+ @VisibleForTesting
+ boolean renameMetaDataFile(FileSystem fs, Path tempMetaDataFile, Path
finalMetaDataFile)
+ throws IOException {
+ return fs.rename(tempMetaDataFile, finalMetaDataFile);
+ }
+
+ @VisibleForTesting
+ boolean renameMetaDataFileAndCheck(FileSystem fs, Path tempMetaDataFile,
Path finalMetaDataFile) {
+ try {
+ // The most important step. There must be no mistakes in this step.
+ // Even if it does, we should stop everything.
Review Comment:
Comments don't make sense here. "Even if it does" What?
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
