[#434] feat(CI): Graviton Trino connector E2E testing #616

xunliu · 2023-10-27T02:55:26Z

What changes were proposed in this pull request?

Add HiveContainer and TrinoContainer
Use Trino JDBC to connect TrinoContainer execute create database, table, insert, select and join two tables test.

Why are the changes needed?

Through Trino operations hive
Supports Trino E2E testing in the local and GitHub Action.

Fix: #434

Does this PR introduce any user-facing change?

N/A

How was this patch tested?

Add TrinoConnectorIT in the Integration-test

github-actions · 2023-10-27T02:57:31Z

Code Coverage Report

Overall Project	67.17%	🟢

There is no coverage information present for the Files changed

...-test/src/test/java/com/datastrato/gravitino/integration/test/trino/AutoCloseableCloser.java

...ation-test/src/test/java/com/datastrato/gravitino/integration/test/trino/TrinoContainer.java

...ion-test/src/test/java/com/datastrato/gravitino/integration/test/trino/TrinoConnectorIT.java

jerryshao · 2023-10-27T05:38:47Z

.github/workflows/integration-test.yml

-        if: ${{ contains(github.event.pull_request.labels.*.name, 'build docker image') }}
-        run: ./dev/docker/hive/build-docker.sh --platform ${PLATFORM} --image ${HIVE_IMAGE_NAME}:${HIVE_IMAGE_TAG_NAME}
+      - name: Pulling the Docker image in advance
+        if: ${{ !contains(github.event.pull_request.labels.*.name, 'build docker image') }}


Is this label still useful? I found that you removed docker run logic, so is this tag still meaningful?

I rollback these changes.

jerryshao · 2023-10-27T05:41:44Z

.github/workflows/integration-test.yml


      - name: Run AMD64 container
        run: |
-          docker run --rm --name ${DOCKER_RUN_NAME} --platform ${PLATFORM} -d -p 8022:22 -p 8088:8088 -p 9000:9000 -p 9083:9083 -p 10000:10000 -p 10002:10002 -p 50010:50010 -p 50070:50070 -p 50075:50075 ${HIVE_IMAGE_NAME}:${HIVE_IMAGE_TAG_NAME}
+          docker run --rm --name ${DOCKER_RUN_NAME} --platform ${PLATFORM} -d -p 8022:22 -p 8088:8088 -p 9000:9000 -p 9083:9083 -p 10000:10000 -p 10002:10002 -p 50010:50010 -p 50070:50070 -p 50075:50075 ${HIVE_IMAGE_NAME}


I don't see trino dock run command, is it required?

Trino container will auto-running in the integration test. so we didn't execute Trino docker run command.

jerryshao · 2023-10-27T05:43:00Z

dev/docker/tools/macos-docker-connector.sh

+# Copyright 2023 Datastrato.
+# This software is licensed under the Apache License version 2.
+#
+#set -ex


What is the usage of this script, can you please add some comments to explain the usage scenario of this script.

OK, I added README.md in the dev/docker/tools/ directory.

dev/docker/trino/conf/catalog/hive.properties.template

jerryshao · 2023-10-27T05:52:29Z

docs/integration-test.md

+Because Docker Desktop for Mac does not provide access to container IP from host(macOS). 
+The macos-docker-connector provides the ability for the macOS host to directly access the docker container IP. 
+Before running the integration tests, make sure to execute the `dev/docker/tools/macos-docker-connector.sh` script.
+
 #### Running Gravitino Hive CI Docker Environment

 1. Run a hive docker test environment container in the local using the `docker run --rm -d -p 8022:22 -p 8088:8088 -p 9000:9000 -p 9083:9083 -p 10000:10000 -p 10002:10002 -p 50010:50010 -p 50070:50070 -p 50075:50075 datastrato/gravitino-ci-hive` command.


Can you please add the doc about how to run trino integration test locally.

OK, I updated you about how to run the Trino integration test in the docs/integration-test.md, Actual TrinoIT will auto-check the test environment. If you didn't run mac-docker-connector in your local, then TrinoIT will not be running.

So you have to change the paragraph title. Here it only mentions the "Hive CI Docker". Another thing is that do we need to start a docker before running the integration test?

I think you should reorganize the doc for others without background easy to run and hide details as much as you can.

jerryshao · 2023-10-27T05:53:26Z

integration-test/build.gradle.kts

    doFirst {
+      copy {
+        from("${project.rootDir}/dev/docker/trino/conf")
+        into("build/tirno-conf")


typo: trino-conf

Still not fixed.

...ration-test/src/test/java/com/datastrato/gravitino/integration/test/trino/BaseContainer.java

jerryshao · 2023-10-27T06:09:12Z

...ion-test/src/test/java/com/datastrato/gravitino/integration/test/trino/TrinoConnectorIT.java

+  }
+
+  @Test
+  public void joinTwoHiveTableTest() throws TException, InterruptedException {


Can you please split into several test, for each test only cover one functionality, which will be easy to maintain and add more tests later on.

...ion-test/src/test/java/com/datastrato/gravitino/integration/test/trino/TrinoConnectorIT.java

.github/workflows/integration-test.yml

docs/integration-test.md

...ration-test/src/test/java/com/datastrato/gravitino/integration/test/trino/BaseContainer.java

...ion-test/src/test/java/com/datastrato/gravitino/integration/test/trino/TrinoConnectorIT.java

dev/docker/trino/conf/log.properties

...ration-test/src/test/java/com/datastrato/gravitino/integration/test/trino/BaseContainer.java

xunliu · 2023-10-31T10:51:05Z

hi @jerryshao @yuqi1129 @mchades I improved this PR, Please help me review it, Thanks.

...ation-test/src/test/java/com/datastrato/gravitino/integration/test/trino/TrinoContainer.java

...ration-test/src/test/java/com/datastrato/gravitino/integration/test/trino/BaseContainer.java

dev/docker/hive/README.md

...ion-test/src/test/java/com/datastrato/gravitino/integration/test/trino/TrinoConnectorIT.java

xunliu · 2023-11-01T03:17:36Z

@jerryshao I have fixed all the comments issue, Please help me review this PR, thanks.

jerryshao · 2023-11-01T16:28:31Z

dev/docker/tools/README.md

+Because Docker Desktop for Mac does not provide access to container IP from host(macOS).
+The [mac-docker-connector](https://github.com/wenjunxiao/mac-docker-connector) provides the ability for the macOS host to directly access the docker container IP.
+This can result in host(macOS) and containers not being able to access each other's internal services directly over IPs.


The words here are confusing to me. I think it might be:

Because Docker Desktop for Mac does not provide access to container IP from host(macOS). This can result in host(macOS) and containers not being able to access each other's internal services directly over IPs. The [mac-docker-connector](https://github.com/wenjunxiao/mac-docker-connector) provides the ability for the macOS host to directly access the docker container IP.

OK, I fixed it.

jerryshao · 2023-11-01T17:24:50Z

docs/integration-test.md

+------------------- Check Docker environment ------------------
+Docker server status .......................................... [running]
+Gravitino IT Docker container is already running ............... [no]
+Run only test cases where tag is set `gravitino-trino-it`.      [embbeded|deploy test]


I found that the gradle output doesn't match the description you mentioned here.

I have tested in these case:

No Gravitino-hive-ci docker container and mac-docker-connector running.

------------------ Check Docker environment ----------------- Docker server status ........................................ [running] Gravitino IT Docker container is already running ............. [no] Run test cases without `gravitino-tirno-it` tag .............. [embedded test] Run test cases without `gravitino-docker-it` tag ............. [embedded test] -------------------------------------------------------------

Only running Gravitino-hive-ci docker container

------------------ Check Docker environment ----------------- Docker server status ........................................ [running] Gravitino IT Docker container is already running ............. [yes] Run test cases without `gravitino-tirno-it` tag .............. [embedded test] Use Gravitino IT Docker container to run all integration test. [embedded test] -------------------------------------------------------------

Running Gravitino-hive-ci docker container and mac-docker-connector

------------------ Check Docker environment ----------------- Docker server status ........................................ [running] Gravitino IT Docker container is already running ............. [yes] Use Gravitino IT Docker container to run all integration test. [embedded test] -------------------------------------------------------------

Other, I will reconstruction HiveCatalogIT code and document in the #639

OK, I see, but there's a typo "tirno".

OK, I fixed it.

jerryshao · 2023-11-01T17:32:00Z

docs/integration-test.md

+Because Docker Desktop for Mac does not provide access to container IP from host(macOS). 
+The macos-docker-connector provides the ability for the macOS host to directly access the docker container IP. 
+Before running the integration tests, make sure to execute the `dev/docker/tools/macos-docker-connector.sh` script.
+
 #### Running Gravitino Hive CI Docker Environment

 1. Run a hive docker test environment container in the local using the `docker run --rm -d -p 8022:22 -p 8088:8088 -p 9000:9000 -p 9083:9083 -p 10000:10000 -p 10002:10002 -p 50010:50010 -p 50070:50070 -p 50075:50075 datastrato/gravitino-ci-hive` command.


So you have to change the paragraph title. Here it only mentions the "Hive CI Docker". Another thing is that do we need to start a docker before running the integration test?

I think you should reorganize the doc for others without background easy to run and hide details as much as you can.

jerryshao · 2023-11-01T17:34:18Z

integration-test/build.gradle.kts

@@ -129,13 +135,20 @@ fun printDockerCheckInfo() {
  }
  val dockerRunning = project.extra["dockerRunning"] as? Boolean ?: false
  val hiveContainerRunning = project.extra["hiveContainerRunning"] as? Boolean ?: false
+  val macDockerConnector = project.extra["macDockerConnector"] as? Boolean ?: false
  if (dockerRunning && hiveContainerRunning) {
    EXCLUDE_DOCKER_TEST = false


You'd better change to "EXCLUDE_HIVE_TEST".

I'm going to refactor the HiveCatalogIT code and documentation in #639 and we won't need to check the container environment anymore!

jerryshao · 2023-11-01T17:35:42Z

integration-test/build.gradle.kts

  if (dockerRunning && hiveContainerRunning) {
    EXCLUDE_DOCKER_TEST = false
  }
+  if (dockerRunning && macDockerConnector) {
+    EXCLUDE_TRINO_TEST = false
+  }

  println("------------------ Check Docker environment -----------------")
  println("Docker server status ........................................ [${if (dockerRunning) "running" else "stop"}]")
  println("Gravitino IT Docker container is already running ............. [${if (hiveContainerRunning) "yes" else "no"}]")


It's actually hive container, you need to change to make people easy to understand.

In the next PR #639, we simply check to see if Docker is running. the Hive and Trino Docker containers will start automatically in the integration test code.

jerryshao · 2023-11-01T17:36:34Z

integration-test/build.gradle.kts

    doFirst {
+      copy {
+        from("${project.rootDir}/dev/docker/trino/conf")
+        into("build/tirno-conf")


Still not fixed.

jerryshao · 2023-11-01T17:49:17Z

...ration-test/src/test/java/com/datastrato/gravitino/integration/test/trino/BaseContainer.java

+    String ipAddress = containerResponse.getNetworkSettings().getIpAddress();
+    Map<String, ContainerNetwork> containerNetworkMap =
+        containerResponse.getNetworkSettings().getNetworks();
+    Assertions.assertEquals(1, containerNetworkMap.size());


It's not proper to use Assertions here for non-test code. You can change to use Guava Preconditions instead.

Ok, I fixed it.

jerryshao · 2023-11-01T17:58:32Z

...ration-test/src/test/java/com/datastrato/gravitino/integration/test/trino/HiveContainer.java

+ * Copyright 2023 Datastrato.
+ * This software is licensed under the Apache License version 2.
+ */
+package com.datastrato.gravitino.integration.test.trino;


Here it is still the same: a) it is not proper to put HiveContainer code to trino package. b) this is non-test code, better moving to src utils package.

Good point! I fixed it.

jerryshao · 2023-11-01T17:59:47Z

...ration-test/src/test/java/com/datastrato/gravitino/integration/test/trino/HiveContainer.java

+    private Builder() {
+      this.image = DEFAULT_IMAGE;
+      this.hostName = HOST_NAME;
+      this.exposePorts = ImmutableSet.of(8088, 9000, 9083, 10000, 10002, 50070, 50075, 50010);


You'd better clearly define the port number in code, not just a hard-coded integer here.

...test/src/test/java/com/datastrato/gravitino/integration/test/trino/PrintingContainerLog.java

jerryshao · 2023-11-01T18:07:27Z

...ion-test/src/test/java/com/datastrato/gravitino/integration/test/trino/TrinoConnectorIT.java

+import org.testcontainers.containers.Network;
+
+@Tag("gravitino-trino-it")
+public class TrinoConnectorIT extends AbstractIT {


I think you need to refactor this class a bit.

Move the prepare-to-test and check-status code to an abstract class. With this test class, it is better only having test code.

Test the functionalities one by one. For example, 1) operate schemas, like create, list, alter, drop...; b) operate tables; c) operate data.

Here your test is actually a scenario test, not a functional test.

Also it is hard to add new functional validation to your code if Trino connector supports more functions.

Good point.

Move the prepare-to-test and check-status code to an abstract class. With this test class, it is better only having test code.

DONE.

Test the functionalities one by one. For example, 1) operate schemas, like create, list, alter, drop...; b) operate tables; c) operate data.

Trino supports many SQL, We need to add test case to cover that.
I think the best way to verify our trino-connector is to use the official Trino test cases in Gravitino.
So I created #657 to track this issue.

integration-test/build.gradle.kts

xunliu · 2023-11-02T11:53:53Z

@jerryshao I fixed all the issues about comments, Please help me review this PR, Thanks.

jerryshao · 2023-11-03T04:28:14Z

...ion-test/src/test/java/com/datastrato/gravitino/integration/test/trino/TrinoConnectorIT.java

+
+  @Test
+  @Order(2)
+  public void createTableTest() throws TException, InterruptedException {


Can you please rename all the test method name to "testXXX"

jerryshao · 2023-11-03T04:30:19Z

integration-test/src/test/resources/logback-test.xml

+
+  <logger name="org.testcontainers" level="DEBUG"/>
+  <logger name="com.github.dockerjava" level="WARN"/>
+</configuration>


What is the usage of this file?

I deleted this unused logback-test.xml

xunliu · 2023-11-03T07:05:15Z

@jerryshao I fixed the 2 issues you raised. Please review it again. Thanks!

### What changes were proposed in this pull request? 1. Add HiveContainer and TrinoContainer 3. Use Trino JDBC to connect TrinoContainer execute create database, table, insert, select and join two tables test. ### Why are the changes needed? + Through Trino operations hive + Supports Trino E2E testing in the local and GitHub Action. Fix: #434 ### Does this PR introduce _any_ user-facing change? N/A ### How was this patch tested? Add TrinoConnectorIT in the Integration-test

[#434] Graviton CI testing Trino Docker

69556df

xunliu requested review from jerryshao, FANNG1, diqiu50, yuqi1129 and mchades October 27, 2023 02:55

yuqi1129 reviewed Oct 27, 2023

View reviewed changes

xunliu added the branch-0.2 label Oct 27, 2023

jerryshao reviewed Oct 27, 2023

View reviewed changes

yuqi1129 reviewed Oct 27, 2023

View reviewed changes

...ion-test/src/test/java/com/datastrato/gravitino/integration/test/trino/TrinoConnectorIT.java Outdated Show resolved Hide resolved

...ion-test/src/test/java/com/datastrato/gravitino/integration/test/trino/TrinoConnectorIT.java Outdated Show resolved Hide resolved

mchades mentioned this pull request Oct 27, 2023

[#554] improvement(CI): improve Hadoop access permission #620

Closed

mchades reviewed Oct 30, 2023

View reviewed changes

.github/workflows/integration-test.yml Outdated Show resolved Hide resolved

yuqi1129 reviewed Oct 30, 2023

View reviewed changes

mchades assigned xunliu Oct 31, 2023

mchades reviewed Oct 31, 2023

View reviewed changes

dev/docker/trino/conf/log.properties Outdated Show resolved Hide resolved

mchades reviewed Oct 31, 2023

View reviewed changes

...ration-test/src/test/java/com/datastrato/gravitino/integration/test/trino/BaseContainer.java Outdated Show resolved Hide resolved

improve code

8311ed0

yuqi1129 reviewed Oct 31, 2023

View reviewed changes

...ion-test/src/test/java/com/datastrato/gravitino/integration/test/trino/TrinoConnectorIT.java Outdated Show resolved Hide resolved

xunliu added 2 commits October 31, 2023 22:36

fix

0e867a7

Fixed checkTrinoContainerStatus()

d355c6c

jerryshao reviewed Nov 1, 2023

View reviewed changes

integration-test/build.gradle.kts Outdated Show resolved Hide resolved

Improve code

ea76b6d

jerryshao reviewed Nov 3, 2023

View reviewed changes

Delete the unused logback-test.xml file.

234fad1

jerryshao approved these changes Nov 3, 2023

View reviewed changes

jerryshao merged commit 8cf2f61 into apache:main Nov 3, 2023

[#434] feat(CI): Graviton Trino connector E2E testing #616

[#434] feat(CI): Graviton Trino connector E2E testing #616

Conversation

xunliu commented Oct 27, 2023

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

github-actions bot commented Oct 27, 2023 • edited Loading

Code Coverage Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xunliu Oct 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xunliu commented Oct 31, 2023

xunliu commented Nov 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xunliu Nov 2, 2023 • edited Loading

Choose a reason for hiding this comment

xunliu commented Nov 2, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xunliu commented Nov 3, 2023

github-actions bot commented Oct 27, 2023 •

edited

Loading

xunliu Oct 31, 2023 •

edited

Loading

xunliu Nov 2, 2023 •

edited

Loading