ChristinaTech commented on code in PR #7198:
URL: https://github.com/apache/iceberg/pull/7198#discussion_r1153794647
##########
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:
You are correct that the goal of this PR is to make the situation this test
replicates impossible. This test was created for 3 reasons:
* As a way of reliability reproducing the issue experienced in production
* As a form of monitor to confirm that the issue can still occur if the
`RetryDetector` mechanism introduced is not made aware of the retry
* As a test of correct behavior from the `RetryDetector`. Though this could
and arguably should be accomplished via separate set of unit tests for the
`RetryDetector`, which I think I will add in a follow-up commit regardless of
the outcome of this conversation.
If AWS Glue service or AWS SDK behavior changes such that this change is no
longer necessary to prevent this failure mode from occurring (such as Glue
making this API transparently idempotent), then this test would fail and we
would know at that point we needed to re-evaluate the mechanism.
While I plan to address the 3rd reason via a unit test, I still believe the
second reason in particular provides ongoing value as an integ test when
dealing with software components we don't control.
--
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]
