Skip to content

[FLINK-37733] Externalise DynamoDB connector IT Test to E2E test package #203

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[FLINK-37733] Externalise DynamoDB connector IT Test to E2E test package #203

wants to merge 1 commit into from

Conversation

darenwkt
Copy link
Contributor

Purpose of the change

[FLINK-37733] Externalise DynamoDB connector IT Test to E2E test package

  • DynamoDB connector IT Test currently resides in flink-connector-dynamodb package which means:
    • It is inconsistent with other connector package where IT test resides in their own e2e test package.
    • Building the package takes longer as IT test takes time to spin up a docker environment for the test.
    • Github action/workflow does not run the IT test as they are not in e2e test package.
  • This is a minor improvement to relocate the IT test into its own e2e test package similar to other connectors

Verifying this change

Please make sure both new and modified tests in this PR follows the conventions defined in our code quality guide: https://flink.apache.org/contributing/code-style-and-quality-common.html#testing

  • This change is a trivial rework / code cleanup without any test coverage as it just relocates existing IT test to a new E2E test package
  • Verified the e2e test passes as follows:
    • Run mvn clean verify -Prun-end-to-end-tests -DdistDir=/Users/Downloads/flink-1.19.2
[INFO] -------------------------------------------------------
[INFO]  T E S T S
[INFO] -------------------------------------------------------
[INFO] Running org.apache.flink.connector.dynamodb.sink.test.DynamoDbSinkITCase
[INFO] Tests run: 6, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 114.11 s - in org.apache.flink.connector.dynamodb.sink.test.DynamoDbSinkITCase
[INFO] Running org.apache.flink.connector.dynamodb.table.test.DynamoDbDynamicSinkITCase
[INFO] Tests run: 2, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 52.551 s - in org.apache.flink.connector.dynamodb.table.test.DynamoDbDynamicSinkITCase
[INFO] 
[INFO] Results:
[INFO] 
[INFO] Tests run: 8, Failures: 0, Errors: 0, Skipped: 0
[INFO] 
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  02:49 min

Significant changes

(Please check any boxes [x] if the answer is "yes". You can first publish the PR and check them afterwards, for convenience.)

  • Dependencies have been added or upgraded
  • Public API has been changed (Public API is any class annotated with @Public(Evolving))
  • Serializers have been changed
  • New feature has been introduced
    • If yes, how is this documented? (not applicable / docs / JavaDocs / not documented)

darenwkt
darenwkt commented Apr 26, 2025
View reviewed changes
...est/java/org/apache/flink/connector/dynamodb/table/RowDataToAttributeValueConverterTest.java Outdated
@@ -216,7 +216,7 @@ void testFloat() {
Map<String, AttributeValue> actualResult =
rowDataToAttributeValueConverter.convertRowData(createElement(value));
Map<String, AttributeValue> expectedResult =
singletonMap(key, AttributeValue.builder().n("1.23456791E17").build());
singletonMap(key, AttributeValue.builder().n("1.2345679E17").build());
Copy link
Contributor Author

@darenwkt darenwkt Apr 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated this test case as it failed locally when I ran it with Java version 8.0.432-amzn (Corretto) due to Java floating point number precision only accurate up to 6-7 digits decimal place.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we use java11 right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we use java11 right?

We use Java 8 based on https://repo1.maven.org/maven2/org/apache/flink/flink-connector-parent/1.0.0/flink-connector-parent-1.0.0.pom

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @gguptp I have revisited this and found that I was running the test with java21 locally, once I changed it to java8, the test passed. As such I have reverted changes to this test case. Please take another look

leekeiabstraction
leekeiabstraction reviewed May 19, 2025
View reviewed changes
flink-connector-aws-e2e-tests/flink-connector-dynamodb-e2e-tests/pom.xml Outdated
Comment on lines 105 to 109
<dependency>
<groupId>software.amazon.awssdk</groupId>
<artifactId>s3</artifactId>
<scope>test</scope>
</dependency>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you elaborate why s3 dependency is needed here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@leekeiabstraction leekeiabstraction leekeiabstraction left review comments

@gguptp gguptp gguptp approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@darenwkt @gguptp @leekeiabstraction