[#639] feat(CI): Improvement Docker management for integration test #711

xunliu · 2023-11-09T14:41:14Z

What changes were proposed in this pull request?

I'm considering referencing the testcontaine approach in TrinoConnectorIT. It can be better to use containers for testing.

Why are the changes needed?

Currently, the HiveCatalogIT and CatalogIcebergIT integrated test environment launches the gravitino-ci-hive Docker image via a script, This creates a situation where the startup container is in GitHub Action or local to the user, and the test container is in the IT code, which is not uniform.

Fix: #639

Does this PR introduce any user-facing change?

N/A

How was this patch tested?

CI Pass

github-actions · 2023-11-09T14:42:42Z

Code Coverage Report

Overall Project	66.13%	🟢

There is no coverage information present for the Files changed

jerryshao · 2023-11-10T07:10:20Z

integration-test/build.gradle.kts

    }
+  } catch (e: Exception) {
+    println("isMacDockerConnectorRunning execution failed: ${e.message}")


Probably be better change to “checkContainerRunning command execution failed: ...”

jerryshao · 2023-11-10T07:14:14Z

integration-test/build.gradle.kts

+    if (exitCode == 0) {
+      project.extra["dockerRunning"] = true
+    } else {
+      println("checkDockerRunning Command execution failed with exit code $exitCode")


Low case "c" for "Command"

jerryshao · 2023-11-10T07:18:45Z

...test/src/test/java/com/datastrato/gravitino/integration/test/catalog/hive/CatalogHiveIT.java

+                    containerSuite.getHiveContainer().getContainerIpAddress(),
+                    HiveContainer.HDFS_DEFAULTFS_PORT))
+            .config(
+                "spark.hadoop.fs.defaultFS",


Why do we need to set this configuration.

I removed it.

jerryshao · 2023-11-10T07:29:06Z

...test/src/test/java/com/datastrato/gravitino/integration/test/catalog/hive/CatalogHiveIT.java

+    try {
+      closer.close();
+    } catch (Exception e) {
+      LOG.error(e.getMessage(), e);


e.getMessage() should be simlar to e, I think we should add some more words for error message.

jerryshao · 2023-11-10T07:31:56Z

...test/src/test/java/com/datastrato/gravitino/integration/test/catalog/hive/CatalogHiveIT.java

@@ -446,7 +482,10 @@ public void testHiveTableProperties() throws TException, InterruptedException {
                    TABLE_TYPE,
                    "external_table",
                    LOCATION,
-                    "/tmp",
+                    String.format(
+                        "hdfs://%s:%d/tmp",


Is it better to use a random path?

This is the basic path of the Table, all tables have different table name. so they don't clash.

jerryshao · 2023-11-10T07:45:12Z

I think you should also update the integration test doc.

...n-test/src/test/java/com/datastrato/gravitino/integration/test/container/TrinoContainer.java

yuqi1129 · 2023-11-10T07:58:35Z

@xunliu
If we do not start Docker locally, there will be a problem. Please take a look.

yuqi1129 · 2023-11-13T02:27:19Z

Additionally, I suggest we do not remove messages explaining why some tests were ignored when testing them in IDE,

New developers would be very confused.

xunliu · 2023-11-13T09:46:26Z

@yuqi1129 Please help me review this PR again. Thanks

jerryshao · 2023-11-15T11:40:19Z

@xunliu If we do not start Docker locally, there will be a problem. Please take a look.

Is it fixed now? @xunliu

jerryshao · 2023-11-16T03:08:43Z

@xunliu If we do not start Docker locally, there will be a problem. Please take a look.

Is it fixed now? @xunliu

Is it ready for review? @xunliu

xunliu · 2023-11-16T03:18:33Z

@jerryshao I'm running tests with different CPU architectures and if they all pass, I'll update this PR status.

xunliu · 2023-11-29T14:06:29Z

I think you should also update the integration test doc.

DONE

FANNG1 · 2023-11-29T14:11:11Z

...om/datastrato/gravitino/integration/test/catalog/lakehouse/iceberg/IcebergRESTServiceIT.java

 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.TestInstance;
 import org.junit.jupiter.api.TestInstance.Lifecycle;
 import org.junit.jupiter.api.condition.EnabledIf;

 @TestInstance(Lifecycle.PER_CLASS)
+@Tag("gravitino-docker-it")


IcebergRESTServiceIT doesn't need @Tag("gravitino-docker-it")

hi @FANNG1 Because IcebergRESTServiceIT extent IcebergRESTServiceBaseIT class.

We need get Hive Docker container IP in IcebergRESTServiceBaseIT class.
The relevant code is here

private static Map<String, String> getIcebergJdbcCatalogConfigs() { ... configMap.put( AuxiliaryServiceManager.GRAVITINO_AUX_SERVICE_PREFIX + IcebergRESTService.SERVICE_NAME + "." + IcebergConfig.CATALOG_WAREHOUSE.getKey(), GravitinoITUtils.genRandomName( String.format( "hdfs://%s:%d/user/hive/warehouse-jdbc-sqlite", containerSuite.getHiveContainer().getContainerIpAddress(), HiveContainer.HDFS_DEFAULTFS_PORT)));

I think it better to add the registerIcebergCatalogConfig() abstract interface in IcebergRESTServiceBaseIT.class, That would solve this problem.

So I created issue #874 to trace this issue, Please fix it(#874) and wait until you have time.

xunliu · 2023-11-29T14:11:30Z

@xunliu If we do not start Docker locally, there will be a problem. Please take a look.

I fixed this issue.

xunliu · 2023-11-29T14:51:32Z

@jerryshao I fixed all problems, Please help me review this PR, Thanks.

jerryshao · 2023-11-30T06:21:57Z

Please rebase the code and test it on Linux environment.

xunliu · 2023-11-30T11:06:02Z

@jerryshao I rebased the code and the test passed in the ubuntu environment.

### What changes were proposed in this pull request? 1. make getCatalogConfig abstract, Hive&Jdbc&Memory implement these interfaces. 2. remove `gravitino-docker-it` tag from IcebergRESTServiceIT. ### Why are the changes needed? After #711, It takes too much time to test IcebergRESTServiceIT, there's no need to start the docker container for the memory catalog Fix: #874 ### Does this PR introduce _any_ user-facing change? no ### How was this patch tested? existing UT

xunliu added this to the Graviton 0.3.0 milestone Nov 9, 2023

xunliu requested review from jerryshao, FANNG1 and yuqi1129 November 9, 2023 14:41

xunliu changed the title ~~[#639] feat(CI): Improving integration testing Docker methodology~~ [#639] feat(CI): Improving IT Docker methodology Nov 9, 2023

xunliu changed the title ~~[#639] feat(CI): Improving IT Docker methodology~~ [#639] feat(CI): Improvement IT Docker methodology Nov 9, 2023

xunliu changed the title ~~[#639] feat(CI): Improvement IT Docker methodology~~ [#639] feat(CI): Improvement IT Docker testing methodology Nov 10, 2023

jerryshao reviewed Nov 10, 2023

View reviewed changes

yuqi1129 reviewed Nov 10, 2023

View reviewed changes

...n-test/src/test/java/com/datastrato/gravitino/integration/test/container/TrinoContainer.java Outdated Show resolved Hide resolved

xunliu mentioned this pull request Nov 15, 2023

[#726] feat(doc): Add document on how to sign and verify releases #729

Merged

jerryshao assigned xunliu Nov 23, 2023

jerryshao removed this from the Gravitino 0.3.0 milestone Nov 23, 2023

xunliu mentioned this pull request Nov 29, 2023

[#588] No need to start Yarn in gravitino docker container #866

Merged

FANNG1 reviewed Nov 29, 2023

View reviewed changes

jerryshao mentioned this pull request Nov 29, 2023

[#872] improvement(all): Enable TrinoIT when docker context is orbstack #873

Merged

jerryshao changed the title ~~[#639] feat(CI): Improvement IT Docker testing methodology~~ [#639] feat(CI): Improvement Docker management for integration test Nov 30, 2023

[#639] feat(CI): Improving integration testing Docker methodology

2da2efe

jerryshao approved these changes Nov 30, 2023

View reviewed changes

jerryshao merged commit bb249d6 into apache:main Nov 30, 2023
2 checks passed

xunliu deleted the issue-639 branch December 12, 2023 06:57

FANNG1 mentioned this pull request Dec 13, 2023

[#874] refactor(lakehouse-iceberg) improve IcebergRESTServiceIT #1143

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#639] feat(CI): Improvement Docker management for integration test #711

[#639] feat(CI): Improvement Docker management for integration test #711

xunliu commented Nov 9, 2023

github-actions bot commented Nov 9, 2023 •

edited

Loading

jerryshao Nov 10, 2023

xunliu Nov 13, 2023

jerryshao Nov 10, 2023

xunliu Nov 13, 2023

jerryshao Nov 10, 2023

xunliu Nov 13, 2023

jerryshao Nov 10, 2023

xunliu Nov 29, 2023

jerryshao Nov 10, 2023

xunliu Nov 13, 2023

jerryshao commented Nov 10, 2023

yuqi1129 commented Nov 10, 2023

yuqi1129 commented Nov 13, 2023

xunliu commented Nov 13, 2023

jerryshao commented Nov 15, 2023

jerryshao commented Nov 16, 2023

xunliu commented Nov 16, 2023

xunliu commented Nov 29, 2023

FANNG1 Nov 29, 2023 •

edited by xunliu

Loading

xunliu Nov 29, 2023

xunliu commented Nov 29, 2023

xunliu commented Nov 29, 2023

jerryshao commented Nov 30, 2023

xunliu commented Nov 30, 2023

[#639] feat(CI): Improvement Docker management for integration test #711

[#639] feat(CI): Improvement Docker management for integration test #711

Conversation

xunliu commented Nov 9, 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 Nov 9, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerryshao commented Nov 10, 2023

yuqi1129 commented Nov 10, 2023

yuqi1129 commented Nov 13, 2023

xunliu commented Nov 13, 2023

jerryshao commented Nov 15, 2023

jerryshao commented Nov 16, 2023

xunliu commented Nov 16, 2023

xunliu commented Nov 29, 2023

FANNG1 Nov 29, 2023 • edited by xunliu Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xunliu commented Nov 29, 2023

xunliu commented Nov 29, 2023

jerryshao commented Nov 30, 2023

xunliu commented Nov 30, 2023

github-actions bot commented Nov 9, 2023 •

edited

Loading

FANNG1 Nov 29, 2023 •

edited by xunliu

Loading