RussellSpitzer commented on code in PR #9546:
URL: https://github.com/apache/iceberg/pull/9546#discussion_r1481744452
##########
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 this is just adding a different Filesystem requirement. We need
atomic "deletes" which i don't think is guaranteed everywhere. 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 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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
