Skip to content

[GLUTEN-9239][CH] [PART-1] Support Java-17 Rmove JNI_OnUnload#9275

Merged
baibaichen merged 1 commit intoapache:mainfrom
baibaichen:feature/jdk17
Apr 16, 2025
Merged

[GLUTEN-9239][CH] [PART-1] Support Java-17 Rmove JNI_OnUnload#9275
baibaichen merged 1 commit intoapache:mainfrom
baibaichen:feature/jdk17

Conversation

@baibaichen
Copy link
Copy Markdown
Contributor

@baibaichen baibaichen commented Apr 9, 2025

What changes were proposed in this pull request?

Remove JNI_OnUnload, since it is no longer needed. using this PR to run gluten-dev ci for testing java-17

(Fixes: #9239)

How was this patch tested?

Using existed UTs

@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 9, 2025

Run Gluten Clickhouse CI on x86

@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 9, 2025

#9239

Comment on lines -169 to -177
JNIEXPORT void JNI_OnUnload(JavaVM * vm, void * /*reserved*/)
{
// manually destroy native in 'nativeDestroyNative' method
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

IIUC, in CH, the unloading actually does nothing?

Then why was the unloading code from Java side strictly required for CH backend in current code base? https://github.com/apache/incubator-gluten/blob/fdd83714067c4a77006d6695b2bd8f6ad9e3ef23/backends-clickhouse/src/main/scala/org/apache/gluten/backendsapi/clickhouse/CHListenerApi.scala#L84-L89

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

No, we actually need unloading. We curently move to Java_org_apache_gluten_vectorized_ExpressionEvaluatorJniWrapper_nativeDestroyNative see #9242

The reason why we do this is openjdk/jdk17u-dev@75a8b7f doesn't allow unload jni for built-in classloader

img_v3_02kv_a7e0c14c-d3e5-49d3-817e-09826dfa3ccg

Copy link
Copy Markdown
Member

@zhztheplayer zhztheplayer Apr 9, 2025

Choose a reason for hiding this comment

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

Great. I think we can remove the Java-side unloading API then? With some minor refactors.

@github-actions
Copy link
Copy Markdown

Run Gluten Clickhouse CI on x86

@github-actions
Copy link
Copy Markdown

Run Gluten Clickhouse CI on x86

@baibaichen
Copy link
Copy Markdown
Contributor Author

run spark 35 backends-clickhouse with java-17

[2025-04-16T09:17:52.334Z] + LD_PRELOAD=/home/jenkins/agent/workspace/gluten/gluten-ci-dev/gluten/libch.so:/usr/java/openjdk-17/lib/libjsig.so
[2025-04-16T09:17:52.334Z] + export 'MAVEN_OPTS=-Xms1g -Xmx8g -Xss128m -XX:ReservedCodeCacheSize=1g -Dfile.encoding=UTF-8 -Djdk.lang.processReaperUseDefaultStackSize=true'
[2025-04-16T09:17:52.334Z] + MAVEN_OPTS='-Xms1g -Xmx8g -Xss128m -XX:ReservedCodeCacheSize=1g -Dfile.encoding=UTF-8 -Djdk.lang.processReaperUseDefaultStackSize=true'
[2025-04-16T09:17:52.334Z] + mvn clean test -Pbackends-clickhouse -Pspark-3.5 -Pjava-17 -Pspark-ut -Pceleborn -Piceberg -Pdelta -Pkafka -Pscala-2.13 -DlogForkedProcessCommand=true -DwildcardSuites=org.apache.gluten '-DargLine=-Dfile.encoding=UTF-8 -XX:+IgnoreUnrecognizedVMOptions --add-opens=java.base/java.lang=ALL-UNNAMED --add-opens=java.base/java.lang.invoke=ALL-UNNAMED --add-opens=java.base/java.lang.reflect=ALL-UNNAMED --add-opens=java.base/java.io=ALL-UNNAMED --add-opens=java.base/java.net=ALL-UNNAMED --add-opens=java.base/java.nio=ALL-UNNAMED --add-opens=java.base/java.util=ALL-UNNAMED --add-opens=java.base/java.util.concurrent=ALL-UNNAMED --add-opens=java.base/java.util.concurrent.atomic=ALL-UNNAMED --add-opens=java.base/jdk.internal.ref=ALL-UNNAMED --add-opens=java.base/jdk.internal.loader=ALL-UNNAMED --add-opens=java.base/sun.nio.ch=ALL-UNNAMED --add-opens=java.base/sun.nio.cs=ALL-UNNAMED --add-opens=java.base/sun.security.action=ALL-UNNAMED --add-opens=java.base/sun.util.calendar=ALL-UNNAMED --add-opens=java.security.jgss/sun.security.krb5=ALL-UNNAMED -Djdk.reflect.useDirectMethodHandle=false -Dio.netty.tryReflectionSetAccessible=true -Djdk.lang.processReaperUseDefaultStackSize=true -Dgluten.test.data.path=/home/jenkins/agent/workspace/gluten/gluten-ci-dev/testdata -Dspark.gluten.sql.columnar.libpath=/home/jenkins/agent/workspace/gluten/gluten-ci-dev/gluten/libch.so -Dspark.test.home=/tmp/spark35'

@baibaichen
Copy link
Copy Markdown
Contributor Author

image

@baibaichen baibaichen merged commit 7e40187 into apache:main Apr 16, 2025
8 checks passed
@baibaichen baibaichen deleted the feature/jdk17 branch April 16, 2025 10:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[CH] Support JDK17 for the CH backend

2 participants