[GitHub] [iceberg] JonasJ-ap commented on a diff in pull request #7198: AWS: Check commit status after failed commit if AWS client performed retries

["JonasJ-ap (via GitHub)" <gi...@apache.org>]

JonasJ-ap commented on code in PR #7198:
URL: https://github.com/apache/iceberg/pull/7198#discussion_r1153579660


##########
aws/src/integration/java/org/apache/iceberg/aws/glue/TestGlueCatalogCommitFailure.java:
##########
@@ -123,6 +128,74 @@ public void testConcurrentModificationExceptionDoesNotCheckCommitStatus() {
     Assert.assertEquals("No new metadata files should exist", 2, metadataFileCount(ops.current()));
   }
 
+  @Test
+  public void testCheckCommitStatusAfterRetries() {
+    String namespace = createNamespace();
+    String tableName = createTable(namespace);
+    TableIdentifier tableId = TableIdentifier.of(namespace, tableName);
+
+    GlueTableOperations spyOps =
+        Mockito.spy((GlueTableOperations) glueCatalog.newTableOps(tableId));
+    GlueCatalog spyCatalog = Mockito.spy(glueCatalog);
+    Mockito.doReturn(spyOps).when(spyCatalog).newTableOps(Mockito.eq(tableId));
+    Table table = spyCatalog.loadTable(tableId);
+
+    TableMetadata metadataV1 = spyOps.current();
+    simulateRetriedCommit(spyOps, true);
+    updateTable(table, spyOps);
+
+    Assert.assertNotEquals("Current metadata should have changed", metadataV1, spyOps.current());
+    Assert.assertTrue("Current metadata should still exist", metadataFileExists(spyOps.current()));
+    Assert.assertEquals(
+        "No new metadata files should exist", 2, metadataFileCount(spyOps.current()));
+  }
+
+  @Test
+  public void testNoRetryAwarenessCorruptsTable() {
+    // This test exists to replicate the issue the prior test validates the fix for

Review Comment:
   Shall we put the link to the issue (https://github.com/apache/iceberg/issues/7151) here? I think it can help others understand the purpose of these tests better?



##########
aws/src/integration/java/org/apache/iceberg/aws/glue/TestGlueCatalogCommitFailure.java:
##########
@@ -123,6 +128,74 @@ public void testConcurrentModificationExceptionDoesNotCheckCommitStatus() {
     Assert.assertEquals("No new metadata files should exist", 2, metadataFileCount(ops.current()));
   }
 
+  @Test
+  public void testCheckCommitStatusAfterRetries() {
+    String namespace = createNamespace();
+    String tableName = createTable(namespace);
+    TableIdentifier tableId = TableIdentifier.of(namespace, tableName);
+
+    GlueTableOperations spyOps =
+        Mockito.spy((GlueTableOperations) glueCatalog.newTableOps(tableId));
+    GlueCatalog spyCatalog = Mockito.spy(glueCatalog);
+    Mockito.doReturn(spyOps).when(spyCatalog).newTableOps(Mockito.eq(tableId));
+    Table table = spyCatalog.loadTable(tableId);
+
+    TableMetadata metadataV1 = spyOps.current();
+    simulateRetriedCommit(spyOps, true);
+    updateTable(table, spyOps);
+
+    Assert.assertNotEquals("Current metadata should have changed", metadataV1, spyOps.current());
+    Assert.assertTrue("Current metadata should still exist", metadataFileExists(spyOps.current()));
+    Assert.assertEquals(
+        "No new metadata files should exist", 2, metadataFileCount(spyOps.current()));
+  }
+
+  @Test
+  public void testNoRetryAwarenessCorruptsTable() {
+    // This test exists to replicate the issue the prior test validates the fix for
+    String namespace = createNamespace();
+    String tableName = createTable(namespace);
+    TableIdentifier tableId = TableIdentifier.of(namespace, tableName);
+
+    GlueTableOperations spyOps =
+        Mockito.spy((GlueTableOperations) glueCatalog.newTableOps(tableId));
+    GlueCatalog spyCatalog = Mockito.spy(glueCatalog);
+    Mockito.doReturn(spyOps).when(spyCatalog).newTableOps(Mockito.eq(tableId));
+    Table table = spyCatalog.loadTable(tableId);
+
+    simulateRetriedCommit(spyOps, false);

Review Comment:
   [Question] I was wondering if there are any real-world cases where GlueTableOps may receive a ConcurrentModificationException but the table is still successfully committed without any retries. Or is this simulated failure case solely serving as a control group for the fix implemented in this PR? 
   
   It seems a little redundant to have an integration test solely for replicating the issue that has already been addressed by this PR. Could you please clarify this for me? Thank you!



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