yyanyy commented on code in PR #12718:
URL: https://github.com/apache/iceberg/pull/12718#discussion_r2116858171
##########
aws/src/integration/java/org/apache/iceberg/aws/glue/TestGlueCatalogTable.java:
##########
@@ -457,16 +507,18 @@ public void testCommitTableSkipArchive() {
String tableName = getRandomName();
AwsProperties properties = new AwsProperties();
properties.setGlueCatalogSkipArchive(false);
- glueCatalog.initialize(
+ GlueCatalog glueCatalogWithArchive = new GlueCatalog();
Review Comment:
do you mind sharing why this requires a new `GlueCatalog` creation?
##########
aws/src/integration/java/org/apache/iceberg/aws/dynamodb/TestDynamoDbLockManager.java:
##########
@@ -128,7 +128,10 @@ public void testAcquireOnceMultiProcesses() throws
Exception {
})
.collect(Collectors.toList()))
.get();
- assertThat(results).as("should have only 1 process succeeded in
acquisition").hasSize(1);
+ assertThat(results)
+ .as("should have only 1 process succeeded in 16 parallel acquisitions")
+ .hasSize(16)
Review Comment:
nit - it seems that the original intention was to check there's only one
"true" in the results, but somehow was testing the size instead; I think the
`.containsOnlyOnce(true)` is probably the more important part but checking for
size 16 indeed seems a bit confusing and not necessary; is it possible to
remove the line to check for size?
##########
aws/src/integration/java/org/apache/iceberg/aws/glue/TestGlueCatalogTable.java:
##########
@@ -343,28 +349,72 @@ public void testRenameTableFailsToCreateNewTable() {
public void testRenameTableFailsToDeleteOldTable() {
String namespace = createNamespace();
String tableName = createTable(namespace);
- // delete the old table metadata, so that drop old table will fail
+
+ // Create a spy on the Glue client
+ // Spy will simulate a time-out when trying to drop the old table during
rename after new table
+ // already created
+ GlueClient glueClientSpy = spy(GLUE);
+ doAnswer(
+ invocation -> {
+ DeleteTableRequest originalRequest = invocation.getArgument(0);
+ if (originalRequest.name().equals(tableName)) {
+ DeleteTableRequest requestWithTimeout =
+ originalRequest.toBuilder()
+ // Inject short timeout threshold to the original
request
+ .overrideConfiguration(
+ AwsRequestOverrideConfiguration.builder()
+ .apiCallTimeout(Duration.ofMillis(1))
+ .build())
+ .build();
Review Comment:
this is some quite complicated logic to trigger a failure from Glue, is it
possible to just change this line to throw an exception directly, or that might
not hit the case we are looking to test? It also avoids taking dependency on
`ApiCallTimeoutException.class` on L399/L402 which might change in future
--
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-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
