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: [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]
