Re: [PR] Core: Fixed certain operations failing to add new data files during retries [iceberg]

["rdblue (via GitHub)" <gi...@apache.org>]

rdblue commented on code in PR #9230:
URL: https://github.com/apache/iceberg/pull/9230#discussion_r1417819364


##########
core/src/test/java/org/apache/iceberg/TestFastAppend.java:
##########
@@ -313,6 +313,37 @@ public void testRecoveryWithoutManifestList() {
         metadata.currentSnapshot().allManifests(FILE_IO).contains(newManifest));
   }
 
+  @Test
+  public void testRecoveryWithManualReCommit() {
+    TestTables.TestTableOperations ops = table.ops();
+    ops.failCommits(5);
+
+    AppendFiles append = table.newFastAppend().appendFile(FILE_B);
+    Snapshot pending = append.apply();
+    ManifestFile newManifest = pending.allManifests(FILE_IO).get(0);
+    Assert.assertTrue("Should create new manifest", new File(newManifest.path()).exists());
+
+    Assertions.assertThatThrownBy(append::commit)
+            .isInstanceOf(CommitFailedException.class)
+            .hasMessage("Injected failure");
+
+    TableMetadata metadata = readMetadata();
+
+    Assert.assertNull("No snapshot is committed", metadata.currentSnapshot());
+
+    pending = append.apply();
+    newManifest = pending.allManifests(FILE_IO).get(0);
+
+    append.commit();
+
+    metadata = readMetadata();
+    validateSnapshot(null, metadata.currentSnapshot(), FILE_B);
+    Assert.assertTrue("Should commit same new manifest", new File(newManifest.path()).exists());
+    Assert.assertTrue(
+            "Should commit the same new manifest",
+            metadata.currentSnapshot().allManifests(FILE_IO).contains(newManifest));

Review Comment:
   This isn't actually testing that the manifest is reused across commit attempts because it came from `apply` that was called after the latest failure (without a cleanup operation).
   
   Instead of manually exercising this path, why not use a transaction here and fail just once to validate?



-- 
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