BsoBird commented on code in PR #9546:
URL: https://github.com/apache/iceberg/pull/9546#discussion_r1481851042
##########
core/src/main/java/org/apache/iceberg/hadoop/HadoopTableOperations.java:
##########
@@ -289,64 +377,153 @@ Path versionHintFile() {
return metadataPath(Util.VERSION_HINT_FILENAME);
}
- private void writeVersionHint(int versionToWrite) {
+ @VisibleForTesting
+ boolean writeVersionHint(FileSystem fs, Integer versionToWrite) throws
Exception {
Path versionHintFile = versionHintFile();
- FileSystem fs = getFileSystem(versionHintFile, conf);
-
+ Path tempVersionHintFile = metadataPath(UUID.randomUUID() +
"-version-hint.temp");
try {
- Path tempVersionHintFile = metadataPath(UUID.randomUUID().toString() +
"-version-hint.temp");
writeVersionToPath(fs, tempVersionHintFile, versionToWrite);
- fs.delete(versionHintFile, false /* recursive delete */);
- fs.rename(tempVersionHintFile, versionHintFile);
- } catch (IOException e) {
- LOG.warn("Failed to update version hint", e);
+ // We can accept that version Hint fails to write, but we can't accept
that version Hint
+ // writes the wrong version.
+ if (fs.exists(versionHintFile) || findVersionWithOutVersionHint(fs) !=
versionToWrite) {
+ throw new AlreadyExistsException(
+ "Failed to write a new versionHintFile,You are writing to an
old-version-Hint,Are there other clients running in parallel with the current
task?");
+ }
+ return renameVersionHint(fs, tempVersionHintFile, versionHintFile);
+ } catch (Exception e) {
+ // Cleaning up temporary files.
+ io().deleteFile(tempVersionHintFile.toString());
+ throw e;
}
}
- private void writeVersionToPath(FileSystem fs, Path path, int
versionToWrite) throws IOException {
+ @VisibleForTesting
+ void deleteOldVersionHint(FileSystem fs, Path versionHintFile, Integer
nextVersion)
+ throws IOException {
+ // In order to be compatible with scenarios where the iceberg table has
just been created or
Review Comment:
--I think we are also introducing another race condition where another
client can delete the version hint by another process which has just committed
This shouldn't be a problem, if the versionHint is written then the last
commit is actually done. I think it's reasonable for a new commit to delete the
versionHint left over from the previous commit.
--
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
