JonasJ-ap commented on code in PR #7198:
URL: https://github.com/apache/iceberg/pull/7198#discussion_r1154751816
##########
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:
Thank you for your clarification! +1 for unit tests on `RetryDetector`
> 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.
I can now fully understand the purpose of the test. I have a concern
regarding its failure conditions. In general, test failures usually indicate
bugs or errors in code functionality. However, in this particular case, it
seems that even if Glue were to update this API to be idempotent in the future,
the existing mechanism would still work (otherwise
`testCheckCommitStatusAfterRetries` would also fail). This means that the
failure of this test alone does not necessarily indicate that there is
something wrong with the mechanism that needs to be revised, but rather that it
could be improved for better performance.
I think if we want to add this test, we may want to add more descriptive
language to the test and explicitly reflect its purpose in the error message.
This will help clarify the intent of the test and avoid any potential
misunderstandings for other developers who may run this test in the future. For
example, we can have something like the following in the error message for this
test:
```java
// The update to the Glue API has made it no longer necessary to detect
retry and check commit.
// As a result, the table is successfully loaded.
// We may want to consider update the related mechanism in Glue/DynamoDB
Catalog
Assertions.assertThatThrownBy(() -> glueCatalog.loadTable(tableId))
.as("Retry Detection and Commit Check Not Required")
...
```
What do you think?
--
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]
