You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "lliangyu-lin (via GitHub)" <gi...@apache.org> on 2025/04/03 23:22:00 UTC
[PR] AWS: Fix DynamoDB and Glue integration test failures [iceberg]
lliangyu-lin opened a new pull request, #12718:
URL: https://github.com/apache/iceberg/pull/12718
### Description
* Fix integration test failures in aws dynamodb and glue
* Some fixes are directly copied from this [closed PR](https://github.com/apache/iceberg/pull/7234) from @jackye1995
### Testing
```
./gradlew :iceberg-aws:integrationTest --tests "org.apache.iceberg.aws.dynamodb.*"
./gradlew :iceberg-aws:integrationTest --tests "org.apache.iceberg.aws.glue.*"
```
--
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
Re: [PR] AWS: Fix DynamoDB and Glue integration test failures [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #12718:
URL: https://github.com/apache/iceberg/pull/12718#discussion_r2031518904
##########
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:
@lliangyu-lin can you help me understand why the result of this test is changing in size?
--
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
Re: [PR] AWS: Fix DynamoDB and Glue integration test failures [iceberg]
Posted by "lliangyu-lin (via GitHub)" <gi...@apache.org>.
lliangyu-lin commented on code in PR #12718:
URL: https://github.com/apache/iceberg/pull/12718#discussion_r2031567794
##########
aws/src/integration/java/org/apache/iceberg/aws/glue/TestGlueCatalogTable.java:
##########
@@ -343,28 +342,32 @@ 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
+
+ // Simulate table was dropped during rename, where new table already created and need to delete
+ // old table
+ GlueCatalog glueCatalogSpy = Mockito.spy(glueCatalog);
Review Comment:
I agree creating exception in this case is not very ideal. Let me try out using spy to drop the table.
--
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
Re: [PR] AWS: Fix DynamoDB and Glue integration test failures [iceberg]
Posted by "lliangyu-lin (via GitHub)" <gi...@apache.org>.
lliangyu-lin commented on code in PR #12718:
URL: https://github.com/apache/iceberg/pull/12718#discussion_r2031550204
##########
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:
@danielcweeks Thanks for reviewing this!
For the size change, I think this test has always been failing. When running locally, I'm getting following error:
```
[should have only 1 process succeeded in acquisition]
Expected size: 1 but was: 16 in:
[false, true, false, false, ...]
```
Which make sense when looking at how the test is written. 16 tasks are trying to acquire the lock and only 1 will be returning true others are returning false, and we are collecting the boolean result from each task into an array.
```
assertThat(results)
.as("should have only 1 process succeeded in acquisition")
.hasSize(16)
.containsOnce(true)
```
--
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
Re: [PR] AWS: Fix DynamoDB and Glue integration test failures [iceberg]
Posted by "lliangyu-lin (via GitHub)" <gi...@apache.org>.
lliangyu-lin commented on PR #12718:
URL: https://github.com/apache/iceberg/pull/12718#issuecomment-2787153272
@danielcweeks @amogh-jahagirdar I made some updates based on the comments. Could you please help take a look?
--
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
Re: [PR] AWS: Fix DynamoDB and Glue integration test failures [iceberg]
Posted by "lliangyu-lin (via GitHub)" <gi...@apache.org>.
lliangyu-lin commented on code in PR #12718:
URL: https://github.com/apache/iceberg/pull/12718#discussion_r2027881602
##########
aws/src/integration/java/org/apache/iceberg/aws/glue/TestGlueCatalogTable.java:
##########
@@ -333,28 +332,33 @@ public void testRenameTableFailsToCreateNewTable() {
public void testRenameTableFailsToDeleteOldTable() {
Review Comment:
This test is failing because [GlueCatalog dropTable](https://github.com/apache/iceberg/blob/main/aws/src/main/java/org/apache/iceberg/aws/glue/GlueCatalog.java#L338-L347) behavior has changed. It will check whether purge is enabled before refreshing for the current metadata.
Hence, removing the parameter / metadata location from Glue won't cause failures during rename because dropTable will not try to retrieve the metadata and the rename will pass
```
GLUE.updateTable(
UpdateTableRequest.builder()
.databaseName(namespace)
.tableInput(TableInput.builder().name(tableName).parameters(Maps.newHashMap()).build())
.build());
```
--
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
Re: [PR] AWS: Fix DynamoDB and Glue integration test failures [iceberg]
Posted by "yyanyy (via GitHub)" <gi...@apache.org>.
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-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
Re: [PR] AWS: Fix DynamoDB and Glue integration test failures [iceberg]
Posted by "lliangyu-lin (via GitHub)" <gi...@apache.org>.
lliangyu-lin commented on code in PR #12718:
URL: https://github.com/apache/iceberg/pull/12718#discussion_r2027881602
##########
aws/src/integration/java/org/apache/iceberg/aws/glue/TestGlueCatalogTable.java:
##########
@@ -333,28 +332,33 @@ public void testRenameTableFailsToCreateNewTable() {
public void testRenameTableFailsToDeleteOldTable() {
Review Comment:
This test is failing because [GlueCatalog dropTable](https://github.com/apache/iceberg/blob/main/aws/src/main/java/org/apache/iceberg/aws/glue/GlueCatalog.java#L338-L347) behavior has changed. It will check whether it's purge is enabled before getting the refreshing for the current metadata.
Hence, removing the parameter / metadata location from Glue won't cause failures because dropTable will not try to retrieve the metadata and the rename will pass
```
GLUE.updateTable(
UpdateTableRequest.builder()
.databaseName(namespace)
.tableInput(TableInput.builder().name(tableName).parameters(Maps.newHashMap()).build())
.build());
```
--
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
Re: [PR] AWS: Fix DynamoDB and Glue integration test failures [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #12718:
URL: https://github.com/apache/iceberg/pull/12718#discussion_r2031534136
##########
aws/src/integration/java/org/apache/iceberg/aws/glue/TestGlueCatalogTable.java:
##########
@@ -343,28 +342,32 @@ 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
+
+ // Simulate table was dropped during rename, where new table already created and need to delete
+ // old table
+ GlueCatalog glueCatalogSpy = Mockito.spy(glueCatalog);
Review Comment:
I'm a little concerned that we're creating the scenario that we're testing here. Since we're creating the exception and testing for it, it's not clear to me that we're simulating real behavior. Would it be possible to use the spy to drop the table after the new table is created so that the real exception occurs from the client?
--
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
Re: [PR] AWS: Fix DynamoDB and Glue integration test failures [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #12718: AWS: Fix DynamoDB and Glue integration test failures
URL: https://github.com/apache/iceberg/pull/12718
--
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
Re: [PR] AWS: Fix DynamoDB and Glue integration test failures [iceberg]
Posted by "lliangyu-lin (via GitHub)" <gi...@apache.org>.
lliangyu-lin commented on code in PR #12718:
URL: https://github.com/apache/iceberg/pull/12718#discussion_r2029231079
##########
aws/src/integration/java/org/apache/iceberg/aws/glue/TestGlueCatalogLock.java:
##########
@@ -152,7 +152,7 @@ public void testParallelCommitMultiThreadMultiCommit() {
final int currentFilesCount = numCommittedFiles;
Awaitility.await()
.pollInterval(Duration.ofMillis(10))
- .atMost(Duration.ofSeconds(10))
+ .atMost(Duration.ofSeconds(20))
Review Comment:
This is increased due to test flakiness
--
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
Re: [PR] AWS: Fix DynamoDB and Glue integration test failures [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #12718:
URL: https://github.com/apache/iceberg/pull/12718#issuecomment-3021252522
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions.
--
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
Re: [PR] AWS: Fix DynamoDB and Glue integration test failures [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #12718:
URL: https://github.com/apache/iceberg/pull/12718#issuecomment-3050647478
This pull request has been closed due to lack of activity. This is not a judgement on the merit of the PR in any way. It is just a way of keeping the PR queue manageable. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.
--
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
Re: [PR] AWS: Fix DynamoDB and Glue integration test failures [iceberg]
Posted by "lliangyu-lin (via GitHub)" <gi...@apache.org>.
lliangyu-lin closed pull request #12718: AWS: Fix DynamoDB and Glue integration test failures
URL: https://github.com/apache/iceberg/pull/12718
--
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
Re: [PR] AWS: Fix DynamoDB and Glue integration test failures [iceberg]
Posted by "lliangyu-lin (via GitHub)" <gi...@apache.org>.
lliangyu-lin commented on code in PR #12718:
URL: https://github.com/apache/iceberg/pull/12718#discussion_r2031550204
##########
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:
@danielcweeks Thanks for reviewing this!
For the size change, I think this test has always been failing. When running locally, I'm getting following error:
```
[should have only 1 process succeeded in acquisition]
Expected size: 1 but was: 16 in:
[false, true, false, false, ...]
```
Which make sense when looking at how the test is written. 16 tasks are trying to acquire the lock and only 1 will be returning true others are returning false, and we are collecting the boolean result from each task into an array.
```
POOL.submit(
() ->
IntStream.range(0, 16)
.parallel()
.mapToObj(...
```
--
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
Re: [PR] AWS: Fix DynamoDB and Glue integration test failures [iceberg]
Posted by "lliangyu-lin (via GitHub)" <gi...@apache.org>.
lliangyu-lin commented on code in PR #12718:
URL: https://github.com/apache/iceberg/pull/12718#discussion_r2032183554
##########
aws/src/integration/java/org/apache/iceberg/aws/glue/TestGlueCatalogTable.java:
##########
@@ -343,28 +342,32 @@ 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
+
+ // Simulate table was dropped during rename, where new table already created and need to delete
+ // old table
+ GlueCatalog glueCatalogSpy = Mockito.spy(glueCatalog);
Review Comment:
I found a issue in my previous test scenario. [DropTable implementation](https://github.com/apache/iceberg/blob/4bc8ac75efe0d33cc847991b2d99271235e10350/aws/src/main/java/org/apache/iceberg/aws/glue/GlueCatalog.java#L364-L366) will actually swallow the `EntityNotFoundException`, it simply returns false instead of throwing an exception.
This means the test wasn't actually going into the [rollback codepath in rename](https://github.com/apache/iceberg/blob/4bc8ac75efe0d33cc847991b2d99271235e10350/aws/src/main/java/org/apache/iceberg/aws/glue/GlueCatalog.java#L427-L442), which only triggers when an exception is thrown.
I've revised the test to simulate a timeout during the drop table API call instead, which does throw an exception and properly tests the rollback codepath.
--
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
Re: [PR] AWS: Fix DynamoDB and Glue integration test failures [iceberg]
Posted by "lliangyu-lin (via GitHub)" <gi...@apache.org>.
lliangyu-lin commented on code in PR #12718:
URL: https://github.com/apache/iceberg/pull/12718#discussion_r2027881602
##########
aws/src/integration/java/org/apache/iceberg/aws/glue/TestGlueCatalogTable.java:
##########
@@ -333,28 +332,33 @@ public void testRenameTableFailsToCreateNewTable() {
public void testRenameTableFailsToDeleteOldTable() {
Review Comment:
This test is failing because [GlueCatalog dropTable](https://github.com/apache/iceberg/blob/main/aws/src/main/java/org/apache/iceberg/aws/glue/GlueCatalog.java#L338-L347) behavior has changed. It will check whether purge is enabled before getting the refreshing for the current metadata.
Hence, removing the parameter / metadata location from Glue won't cause failures because dropTable will not try to retrieve the metadata and the rename will pass
```
GLUE.updateTable(
UpdateTableRequest.builder()
.databaseName(namespace)
.tableInput(TableInput.builder().name(tableName).parameters(Maps.newHashMap()).build())
.build());
```
--
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