diqiu50 commented on code in PR #7827:
URL: https://github.com/apache/gravitino/pull/7827#discussion_r2239058659
##########
.github/workflows/backend-integration-test-action.yml:
##########
@@ -62,6 +62,12 @@ jobs:
-x :spark-connector:spark-runtime-3.3:test -x
:spark-connector:spark-runtime-3.4:test -x
:spark-connector:spark-runtime-3.5:test
-x :trino-connector:integration-test:test -x
:trino-connector:trino-connector:test
-x :authorizations:authorization-chain:test -x
:authorizations:authorization-ranger:test
+ -x :integration-test:test -x :catalogs:catalog-fileset:test
Review Comment:
Why we remove the module of `:integration-test:test`
##########
.github/workflows/flink-integration-test-action.yml:
##########
@@ -46,9 +46,8 @@ jobs:
- name: Flink Integration Test
id: integrationTest
- # run embedded mode and deploy mode integration tests
+ # run deploy mode integration tests
run: |
- ./gradlew -PskipTests -PtestMode=embedded -PjdkVersion=${{
inputs.java-version }} -PskipDockerTests=false :flink-connector:flink:test
--tests "org.apache.gravitino.flink.connector.integration.test.**"
./gradlew -PskipTests -PtestMode=deploy -PjdkVersion=${{
inputs.java-version }} -PskipDockerTests=false :flink-connector:flink:test
--tests "org.apache.gravitino.flink.connector.integration.test.**"
Review Comment:
Does the Flink integration test not run in deploy mode?
##########
.github/workflows/integration-test-action.yml:
##########
@@ -0,0 +1,73 @@
+name: New Integration Test Action
+
+# run integration test
+on:
+ workflow_call:
+ inputs:
+ architecture:
+ required: true
+ description: 'Architecture of the platform'
+ type: string
+ java-version:
+ required: true
+ description: 'Java version'
+ type: string
+ backend:
+ required: true
+ description: 'Backend storage for Gravitino'
+ type: string
+ test-mode:
+ required: true
+ description: 'run on embedded or deploy mode'
+ type: string
+
+jobs:
+ start-runner:
+ name: JDK${{ inputs.java-version }}-${{ inputs.test-mode }}-${{
inputs.backend }}
+ runs-on: ubuntu-latest
+ timeout-minutes: 90
+ env:
+ PLATFORM: ${{ inputs.architecture }}
+ steps:
+ - uses: actions/checkout@v4
+
+ - uses: actions/setup-java@v4
+ with:
+ java-version: ${{ inputs.java-version }}
+ distribution: 'temurin'
+ cache: 'gradle'
+
+ - name: Set up QEMU
+ uses: docker/setup-qemu-action@v3
+
+ - name: Check required command
+ run: |
+ dev/ci/check_commands.sh
+
+ - name: Package Gravitino
+ if: ${{ inputs.test-mode == 'deploy' }}
Review Comment:
Can we validate this earlier in the process? It currently takes a long time
to fail at this stage.
##########
integration-test-common/src/test/java/org/apache/gravitino/integration/test/util/GravitinoITUtils.java:
##########
@@ -18,12 +18,15 @@
*/
package org.apache.gravitino.integration.test.util;
+import com.google.common.base.Preconditions;
import java.io.File;
import java.nio.charset.StandardCharsets;
import java.util.UUID;
import org.apache.commons.io.FileUtils;
+import org.apache.commons.lang3.StringUtils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+import org.testcontainers.shaded.com.google.common.collect.ImmutableMap;
Review Comment:
Don't use the shaded library
##########
build.gradle.kts:
##########
@@ -105,6 +106,53 @@ project.extra["extraJvmArgs"] = if (extra["jdkVersion"] in
listOf("8", "11")) {
)
}
+fun useHighVersionJDK(project: Project): Boolean {
+ val name = project.name.lowercase()
+ val path = project.path.lowercase()
+
+ // bundles module rely on catalog-fileset module
+ if (name == "catalog-common" || name == "hadoop-common" || name ==
"catalog-fileset") {
+ return false
+ }
+
+ if (path.startsWith(":catalogs:") || path.startsWith(":iceberg:") ||
path.startsWith(":authorizations:")) {
+ return true
+ }
+
+ if (path == ":trino-connector:integration-test" || path ==
":web:integration-test") {
+ return true
+ }
+
+ if (name in listOf("server", "lineage")) {
+ return true
+ }
+
+ // All ITs could only run embedded mode in JDK17
+ if (path.startsWith(":integration-test:") &&
rootProject.extra["isTestModeEmbedded"] == true) {
+ return true
+ }
+
+ return false
+}
+
+// Use JDK 17 for embedded mode, use the JDK from `jdkVersion` for deploy mode
+fun getJdkVersionForTest(project: Project): JavaLanguageVersion {
+ if (rootProject.extra["isTestModeEmbedded"] == true) {
+ return JavaLanguageVersion.of(17)
Review Comment:
It would be better to extract the JDK version into a variable, such as
defaultJdkVersion or minSupportedJdkVersion.
##########
.github/workflows/integration-test-action.yml:
##########
@@ -0,0 +1,73 @@
+name: New Integration Test Action
Review Comment:
Is this integration test running on the client side?
##########
build.gradle.kts:
##########
@@ -105,6 +106,53 @@ project.extra["extraJvmArgs"] = if (extra["jdkVersion"] in
listOf("8", "11")) {
)
}
+fun useHighVersionJDK(project: Project): Boolean {
Review Comment:
is useJDK17 better?
##########
clients/filesystem-hadoop3/src/test/java/org/apache/gravitino/filesystem/hadoop/integration/test/GravitinoVirtualFileSystemIT.java:
##########
@@ -54,10 +54,12 @@
import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.Tag;
import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.condition.DisabledIf;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@Tag("gravitino-docker-test")
+@DisabledIf("org.apache.gravitino.integration.test.util.ITUtils#isEmbedded")
public class GravitinoVirtualFileSystemIT extends BaseIT {
Review Comment:
introduce a base class named ClientIT to avoid adding @DisabledIf
annotations to multiple classes.
##########
integration-test-common/src/test/java/org/apache/gravitino/integration/test/util/GravitinoITUtils.java:
##########
@@ -34,6 +37,9 @@ private GravitinoITUtils() {
public static void startGravitinoServer() {
String gravitinoStartShell = System.getenv("GRAVITINO_HOME") +
"/bin/gravitino.sh";
Review Comment:
Check that it is not null.
##########
iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/integration/test/IcebergRestKerberosHiveCatalogIT.java:
##########
@@ -157,4 +158,20 @@ private static boolean isEmbedded() {
return Objects.equals(mode, ITUtils.EMBEDDED_TEST_MODE);
}
+
+ private static void refreshKerberosConfig() {
+ Class<?> classRef;
+ try {
+ if (System.getProperty("java.vendor").contains("IBM")) {
+ classRef = Class.forName("com.ibm.security.krb5.internal.Config");
+ } else {
+ classRef = Class.forName("sun.security.krb5.Config");
+ }
+
+ Method refershMethod = classRef.getMethod("refresh");
+ refershMethod.invoke(null);
+ } catch (Exception e) {
+ throw new RuntimeException(e);
+ }
+ }
Review Comment:
What problem does this change aim to solve?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
