You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "leesf (via GitHub)" <gi...@apache.org> on 2024/10/18 03:43:22 UTC
[PR] Core: delete temp metadata file when version already exists [iceberg]
leesf opened a new pull request, #11350:
URL: https://github.com/apache/iceberg/pull/11350
(no comment)
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: delete temp metadata file when version already exists [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11350:
URL: https://github.com/apache/iceberg/pull/11350#discussion_r1806163830
##########
core/src/main/java/org/apache/iceberg/hadoop/HadoopTableOperations.java:
##########
@@ -368,7 +368,13 @@ 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);
+ CommitFailedException cfe =
+ new CommitFailedException("Version %d already exists: %s", nextVersion, dst);
+ RuntimeException re = tryDelete(src);
Review Comment:
is it always correct to delete the src file or could there be cases where we'd want to keep it?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: delete temp metadata file when version already exists [iceberg]
Posted by "leesf (via GitHub)" <gi...@apache.org>.
leesf commented on PR #11350:
URL: https://github.com/apache/iceberg/pull/11350#issuecomment-2445898551
> This makes sense. @leesf Could you add a test so we don't break this in the future?
@Fokko Added check to the existing UT with dist file already exists failure.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: delete temp metadata file when version already exists [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra merged PR #11350:
URL: https://github.com/apache/iceberg/pull/11350
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: delete temp metadata file when version already exists [iceberg]
Posted by "leesf (via GitHub)" <gi...@apache.org>.
leesf commented on PR #11350:
URL: https://github.com/apache/iceberg/pull/11350#issuecomment-2462321487
@Fokko gentle ping.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: delete temp metadata file when version already exists [iceberg]
Posted by "leesf (via GitHub)" <gi...@apache.org>.
leesf commented on PR #11350:
URL: https://github.com/apache/iceberg/pull/11350#issuecomment-2421372412
@rdblue please help to review this PR, thanks.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: delete temp metadata file when version already exists [iceberg]
Posted by "leesf (via GitHub)" <gi...@apache.org>.
leesf commented on PR #11350:
URL: https://github.com/apache/iceberg/pull/11350#issuecomment-2449690582
@Fokko I think the PR to good the merge, thanks.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: delete temp metadata file when version already exists [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11350:
URL: https://github.com/apache/iceberg/pull/11350#discussion_r1848408316
##########
core/src/test/java/org/apache/iceberg/hadoop/TestHadoopCommits.java:
##########
@@ -205,6 +205,13 @@ public void testFailedCommit() throws Exception {
List<File> manifests = listManifestFiles();
assertThat(manifests).as("Should contain 0 Avro manifest files").isEmpty();
+
+ // Verifies that there is no temporary metadata.json files left on dist file already exists
+ // failures.
+ Set<String> actual =
+ listMetadataJsonFiles().stream().map(File::getName).collect(Collectors.toSet());
+ Set<String> expected = Sets.newHashSet("v1.metadata.json", "v2.metadata.json");
+ assertThat(actual).as("only v1 and v2 metadata.json should exist.").isEqualTo(expected);
Review Comment:
```suggestion
assertThat(actual).as("only v1 and v2 metadata.json should exist.").containsExactly("v1.metadata.json", "v2.metadata.json");
```
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: delete temp metadata file when version already exists [iceberg]
Posted by "leesf (via GitHub)" <gi...@apache.org>.
leesf commented on code in PR #11350:
URL: https://github.com/apache/iceberg/pull/11350#discussion_r1806229187
##########
core/src/main/java/org/apache/iceberg/hadoop/HadoopTableOperations.java:
##########
@@ -368,7 +368,13 @@ 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);
+ CommitFailedException cfe =
+ new CommitFailedException("Version %d already exists: %s", nextVersion, dst);
+ RuntimeException re = tryDelete(src);
Review Comment:
@nastra yes, i think it is always safe to delete the temp src file(temp metadata file), the commit retry would create a new temp metadata file.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: delete temp metadata file when version already exists [iceberg]
Posted by "leesf (via GitHub)" <gi...@apache.org>.
leesf commented on code in PR #11350:
URL: https://github.com/apache/iceberg/pull/11350#discussion_r1808917408
##########
core/src/main/java/org/apache/iceberg/hadoop/HadoopTableOperations.java:
##########
@@ -368,7 +368,13 @@ 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);
+ CommitFailedException cfe =
+ new CommitFailedException("Version %d already exists: %s", nextVersion, dst);
+ RuntimeException re = tryDelete(src);
Review Comment:
@nastra would you please help to take another look? thanks
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: delete temp metadata file when version already exists [iceberg]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on PR #11350:
URL: https://github.com/apache/iceberg/pull/11350#issuecomment-2445940060
@leesf Thanks! Can you run `./gradlew :iceberg-core:spotlessApply` to fix the formatting? Thanks!
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: delete temp metadata file when version already exists [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11350:
URL: https://github.com/apache/iceberg/pull/11350#discussion_r1848439678
##########
core/src/test/java/org/apache/iceberg/hadoop/TestHadoopCommits.java:
##########
@@ -205,6 +205,13 @@ public void testFailedCommit() throws Exception {
List<File> manifests = listManifestFiles();
assertThat(manifests).as("Should contain 0 Avro manifest files").isEmpty();
+
+ // Verifies that there is no temporary metadata.json files left on dist file already exists
+ // failures.
+ Set<String> actual =
+ listMetadataJsonFiles().stream().map(File::getName).collect(Collectors.toSet());
+ Set<String> expected = Sets.newHashSet("v1.metadata.json", "v2.metadata.json");
+ assertThat(actual).as("only v1 and v2 metadata.json should exist.").isEqualTo(expected);
Review Comment:
```suggestion
// verifies that there is no temporary metadata.json files left on disk
Set<String> actual =
listMetadataJsonFiles().stream().map(File::getName).collect(Collectors.toSet());
assertThat(actual)
.as("only v1 and v2 metadata.json should exist.")
.containsExactly("v1.metadata.json", "v2.metadata.json");
```
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: delete temp metadata file when version already exists [iceberg]
Posted by "leesf (via GitHub)" <gi...@apache.org>.
leesf commented on PR #11350:
URL: https://github.com/apache/iceberg/pull/11350#issuecomment-2485769414
@Fokko hi, anything need to resolve here to merge the PR?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: delete temp metadata file when version already exists [iceberg]
Posted by "leesf (via GitHub)" <gi...@apache.org>.
leesf commented on PR #11350:
URL: https://github.com/apache/iceberg/pull/11350#issuecomment-2446041222
> ./gradlew :iceberg-core:spotlessApply
Done and thanks.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: delete temp metadata file when version already exists [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #11350:
URL: https://github.com/apache/iceberg/pull/11350#discussion_r1848439698
##########
core/src/main/java/org/apache/iceberg/hadoop/HadoopTableOperations.java:
##########
@@ -368,7 +368,13 @@ 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);
+ CommitFailedException cfe =
+ new CommitFailedException("Version %d already exists: %s", nextVersion, dst);
+ RuntimeException re = tryDelete(src);
+ if (re != null) {
+ cfe.addSuppressed(re);
+ }
+ throw cfe;
Review Comment:
nit: newline after block
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: delete temp metadata file when version already exists [iceberg]
Posted by "leesf (via GitHub)" <gi...@apache.org>.
leesf commented on PR #11350:
URL: https://github.com/apache/iceberg/pull/11350#issuecomment-2487135105
@nastra I pushed an update to fix the ut updated by your push.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org