Skip to content
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.

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

[#5071] Improvement (test): Add integration tests for Trino cascading queries #5073

Merged
merged 41 commits into from
Oct 29, 2024

Conversation

diqiu50
Copy link
Contributor

@diqiu50 diqiu50 commented Oct 9, 2024

What changes were proposed in this pull request?

Add integration tests for Trino cascading queries

Why are the changes needed?

Fix: #5071

Does this PR introduce any user-facing change?

NO

How was this patch tested?

New integration testers

@diqiu50 diqiu50 self-assigned this Oct 9, 2024
@diqiu50 diqiu50 changed the title [TEST] Add integration tests for Trino cascading queries [#5071] Improvement (test): Add integration tests for Trino cascading queries Oct 9, 2024
@@ -76,7 +76,7 @@ jobs:

- name: Package Gravitino
run: |
./gradlew compileDistribution -x test -PjdkVersion=${{ matrix.java-version }}
./gradlew build compileDistribution -x test -PjdkVersion=${{ matrix.java-version }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to add the build task? I believe the build task is included in compileDistribution

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the compileDistribution task does not build the Trino connector

#create test metalake
counter=0
while [ $counter -le 30 ]; do
sleep 1
Copy link
Contributor

Choose a reason for hiding this comment

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

Better move this line to L53.

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 need to pause for a moment because the Gravitino server has not completed startup.

cp -r /opt/gravitino-server /tmp/gravitino-server
rm -fr /tmp/gravitino-server/logs
rm -fr /tmp/gravitino-server/data
/tmp/gravitino-server/bin/gravitino.sh start
Copy link
Contributor

Choose a reason for hiding this comment

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

When will you stop the Gravitino server? I haven't noticed such logic in the PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Gravitino server was stopped by the container shutdown.

# under the License.

connector.name=trino
connection-url=jdbc:trino://trino-remote:8080/tpcds
Copy link
Contributor

Choose a reason for hiding this comment

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

${trino-remote-address}:${trino-remote-port}

Copy link
Contributor Author

@diqiu50 diqiu50 Oct 12, 2024

Choose a reason for hiding this comment

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

That URL is a valid connection-url used by the Docker Compose environment.

@diqiu50 diqiu50 requested a review from yuqi1129 October 12, 2024 07:43
@@ -76,7 +76,7 @@ jobs:

- name: Package Gravitino
run: |
./gradlew compileDistribution -x test -PjdkVersion=${{ matrix.java-version }}
./gradlew compileDistribution compileTrinoConnector -x test -PjdkVersion=${{ matrix.java-version }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the task compileTrinoConnector NOT included in compileDistribution?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the compileTrinoConnector task does not include compileDistribution

@diqiu50 diqiu50 closed this Oct 24, 2024
@diqiu50 diqiu50 reopened this Oct 24, 2024
@diqiu50 diqiu50 closed this Oct 24, 2024
@diqiu50 diqiu50 reopened this Oct 24, 2024
@@ -87,6 +87,7 @@ jobs:
run: |
./gradlew -PskipTests -PtestMode=embedded -PjdkVersion=${{ matrix.java-version }} -PskipDockerTests=false :trino-connector:integration-test:test
./gradlew -PskipTests -PtestMode=deploy -PjdkVersion=${{ matrix.java-version }} -PskipDockerTests=false :trino-connector:integration-test:test
trino-connector/integration-test/trino-test-tools/run_test.sh
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the difference between this test script and the above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The script uses a different test set and testing environment to test the Trino connector. It can also specify a custom test set and testing environment for any test cases. Additionally, the above command can be modified to use the script.

Copy link
Contributor

Choose a reason for hiding this comment

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

"a different test set" means tpcds and tpch, right? If so, plz add comment on it otherwise we don't know why run the script individually

Copy link
Contributor Author

@diqiu50 diqiu50 Oct 28, 2024

Choose a reason for hiding this comment

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

"a different test set" means the test set for Trino cascading queries. Now the test set only includes TPCH and TCPDS queries.

}

# Specific functions for each driver or bundle
download_mysql_jar() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Where was this function called?

Copy link
Contributor Author

@diqiu50 diqiu50 Oct 29, 2024

Choose a reason for hiding this comment

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

launch.sh

# Specific functions for each driver or bundle
download_mysql_jar() {
download_jar "mysql-connector-java-8.0.26.jar" \
"https://repo1.maven.org/maven2/mysql/mysql-connector-java/8.0.26/mysql-connector-java-8.0.26.jar" \
Copy link
Contributor

Choose a reason for hiding this comment

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

Once the version is rectified, we can validate its integrity by verifying the MD5 checksum.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There generally shouldn’t be any issues here. If an issue does occur, the download script will raise an error, exit execution, and output the error message.
Let’s fix it later if any issues arise.

Copy link
Contributor

Choose a reason for hiding this comment

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

why not reuse the docker-compose in integration-test-common

Copy link
Contributor Author

Choose a reason for hiding this comment

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

integration-test-common is a single Trino cluster environment; it does not test Trino cascading queries. The new Docker Compose setup includes two Trino clusters to test cascading queries in Trino.

@diqiu50 diqiu50 requested a review from mchades October 29, 2024 02:27
@mchades mchades added the branch-0.7 Automatically cherry-pick commit to branch-0.7 label Oct 29, 2024
@mchades mchades merged commit a384cd8 into apache:main Oct 29, 2024
26 checks passed
github-actions bot pushed a commit that referenced this pull request Oct 29, 2024
… queries (#5073)

### What changes were proposed in this pull request?

Add integration tests for Trino cascading queries

### Why are the changes needed?

Fix: #5071

### Does this PR introduce _any_ user-facing change?

NO

### How was this patch tested?

New integration testers
@diqiu50 diqiu50 deleted the t2t-connector-tests branch October 30, 2024 03:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
branch-0.7 Automatically cherry-pick commit to branch-0.7
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Improvement] Add integration tests for Trino cascading queries
3 participants