fix(iceberg): fix jni context class loader #17478

chenzl25 · 2024-06-27T05:46:43Z

I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.

What's changed and what's your intention?

Try to resolve panic when using Iceberg Sink connector from materialized view #17296

Checklist

I have written necessary rustdoc comments
I have added necessary unit tests and integration tests
I have added test labels as necessary. See details.
I have added fuzzing tests or opened an issue to track them. (Optional, recommended for new SQL features Sqlsmith: Sql feature generation #7934).
My PR contains breaking changes. (If it deprecates some features, please create a tracking issue to remove them in the future).
All checks passed in ./risedev check (or alias, ./risedev c)
My PR changes performance-critical code. (Please run macro/micro-benchmarks and show the results.)

My PR contains critical fixes that are necessary to be merged into the latest release. (Please check out the details)

Documentation

My PR needs documentation updates. (Please use the Release note section below to summarize the impact on users)

Release note

If this PR includes changes that directly affect users or other significant modifications relevant to the community, kindly draft a release note to provide a concise summary of these changes. Please prioritize highlighting the impact these changes will have on users.

wenym1 · 2024-06-27T06:04:04Z

I am thinking that shall we instead always set the context loader after we do attach_current_thread of jni?

chenzl25 · 2024-06-27T06:06:04Z

I am thinking that shall we instead always set the context loader after we do attach_current_thread of jni?

It depends on whether the method needs to use ContextClassLoader. Currently, I only find debezium and iceberg catalog will use it.

wenym1 · 2024-06-27T06:25:53Z

I am thinking that shall we instead always set the context loader after we do attach_current_thread of jni?

It depends on whether the method needs to use ContextClassLoader. Currently, I only find debezium and iceberg catalog will use it.

Java libraries usually assume that the class loader is correctly set. So I think we'd better just always set the class loader when we do attach_current_thread to create a new JniEnv, so that we don't have to manually set the class loader one by one for these methods and can avoid similar exception thrown again when we use other java libraries in the future.

chenzl25 · 2024-06-27T06:59:19Z

I am thinking that shall we instead always set the context loader after we do attach_current_thread of jni?

It depends on whether the method needs to use ContextClassLoader. Currently, I only find debezium and iceberg catalog will use it.

Java libraries usually assume that the class loader is correctly set. So I think we'd better just always set the class loader when we do attach_current_thread to create a new JniEnv, so that we don't have to manually set the class loader one by one for these methods and can avoid similar exception thrown again when we use other java libraries in the future.

Sounds good. Do you have any idea how to implement it?

wenym1 · 2024-06-27T09:05:17Z

I am thinking that shall we instead always set the context loader after we do attach_current_thread of jni?

It depends on whether the method needs to use ContextClassLoader. Currently, I only find debezium and iceberg catalog will use it.

Java libraries usually assume that the class loader is correctly set. So I think we'd better just always set the class loader when we do attach_current_thread to create a new JniEnv, so that we don't have to manually set the class loader one by one for these methods and can avoid similar exception thrown again when we use other java libraries in the future.

Sounds good. Do you have any idea how to implement it?

We can unify all our call to jvm.attach_current_thread() to a method like

fn new_jvm_env(jvm: &JavaVM) -> AttachGuard<'_> {
    let env = jvm.attach_current_thread();
    let thread = call_static_method!(env, {java.lang.Thread.currentThread()});
    let system_class_loader = call_static_method!(env, {java.lang.ClassLoader.getSystemClassLoader()});
    call_method!(env, {void setContextClassLoader(), thread, {system_class_loader}});
    env
}

…ss_loader_for_jni_catalog

chenzl25 · 2024-07-03T09:57:52Z

@wenym1 I finished the refactoring, PTAL again.

chenzl25 · 2024-07-03T10:07:07Z

src/jni_core/src/jvm_runtime.rs

+    // set context class loader for the thread
+    // java.lang.Thread.currentThread()
+    //     .setContextClassLoader(java.lang.ClassLoader.getSystemClassLoader());
+
+    let thread = crate::call_static_method!(
+        env,
+        {Thread},
+        {Thread currentThread()}
+    )?;
+
+    let system_class_loader = crate::call_static_method!(
+        env,
+        {ClassLoader},
+        {ClassLoader getSystemClassLoader()}
+    )?;
+
+    crate::call_method!(
+        env,
+        thread,
+        {void setContextClassLoader(ClassLoader)},
+        &system_class_loader
+    )?;


java.lang.Thread.currentThread().setContextClassLoader(java.lang.ClassLoader.getSystemClassLoader()); would be called once after attaching a thread to jvm successfully and any jni related codes should be wrapped in execute_with_jni_env.

chenzl25 · 2024-07-03T10:08:41Z

src/connector/src/sink/remote.rs

+            if let Err(e) = result {
+                let _ = response_tx.blocking_send(Err(e));
+            }


@wenym1 Do I correctly handle the error in this case?

wenym1

All tests are passed. LGTM

wenym1 · 2024-07-04T06:29:13Z

src/connector/src/sink/remote.rs

+            if let Err(e) = result {
+                let _ = response_tx.blocking_send(Err(e));
+            }


StrikeW · 2024-07-04T06:36:54Z

src/jni_core/src/jvm_runtime.rs

-    let mut env = jvm.get_env().with_context(|| "Failed to get jni env")?;
+    // set context class loader for the thread
+    // java.lang.Thread.currentThread()
+    //     .setContextClassLoader(java.lang.ClassLoader.getSystemClassLoader());


This is the equivalent java code, I think it's for reference purpose.

StrikeW

LGTM

fix context class loader

c439245

github-actions bot added the type/fix Bug fix label Jun 27, 2024

chenzl25 mentioned this pull request Jun 27, 2024

panic when using Iceberg Sink connector from materialized view #17296

Open

chenzl25 requested review from wenym1 and fuyufjh June 27, 2024 05:58

chenzl25 added the user-facing-changes Contains changes that are visible to users label Jun 27, 2024

chenzl25 added ci/run-e2e-iceberg-sink-tests and removed user-facing-changes Contains changes that are visible to users labels Jun 27, 2024

chenzl25 added 2 commits July 3, 2024 16:54

Merge remote-tracking branch 'origin/main' into dylan/fix_context_cla…

d17c5ef

…ss_loader_for_jni_catalog

refactor

f2fdbd3

chenzl25 requested review from StrikeW and BugenZhao July 3, 2024 09:57

chenzl25 added ci/run-java-connector-node-integration-tests ci/run-java-binding-tests ci/run-e2e-source-tests ci/run-e2e-sink-tests ci/run-pulsar-source-tests ci/run-connector-node-integration-tests labels Jul 3, 2024

chenzl25 commented Jul 3, 2024

View reviewed changes

wenym1 approved these changes Jul 4, 2024

View reviewed changes

StrikeW reviewed Jul 4, 2024

View reviewed changes

StrikeW approved these changes Jul 4, 2024

View reviewed changes

chenzl25 added this pull request to the merge queue Jul 4, 2024

Merged via the queue into main with commit 8fb5606 Jul 4, 2024
59 of 60 checks passed

chenzl25 deleted the dylan/fix_context_class_loader_for_jni_catalog branch July 4, 2024 12:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(iceberg): fix jni context class loader #17478

fix(iceberg): fix jni context class loader #17478

chenzl25 commented Jun 27, 2024

wenym1 commented Jun 27, 2024

chenzl25 commented Jun 27, 2024 •

edited

Loading

wenym1 commented Jun 27, 2024

chenzl25 commented Jun 27, 2024

wenym1 commented Jun 27, 2024

chenzl25 commented Jul 3, 2024

chenzl25 Jul 3, 2024

chenzl25 Jul 3, 2024

wenym1 Jul 4, 2024

wenym1 left a comment

wenym1 Jul 4, 2024

StrikeW Jul 4, 2024

wenym1 Jul 4, 2024

StrikeW left a comment

fix(iceberg): fix jni context class loader #17478

fix(iceberg): fix jni context class loader #17478

Conversation

chenzl25 commented Jun 27, 2024

What's changed and what's your intention?

Checklist

Documentation

Release note

wenym1 commented Jun 27, 2024

chenzl25 commented Jun 27, 2024 • edited Loading

wenym1 commented Jun 27, 2024

chenzl25 commented Jun 27, 2024

wenym1 commented Jun 27, 2024

chenzl25 commented Jul 3, 2024

chenzl25 Jul 3, 2024

Choose a reason for hiding this comment

chenzl25 Jul 3, 2024

Choose a reason for hiding this comment

wenym1 Jul 4, 2024

Choose a reason for hiding this comment

wenym1 left a comment

Choose a reason for hiding this comment

wenym1 Jul 4, 2024

Choose a reason for hiding this comment

StrikeW Jul 4, 2024

Choose a reason for hiding this comment

wenym1 Jul 4, 2024

Choose a reason for hiding this comment

StrikeW left a comment

Choose a reason for hiding this comment

chenzl25 commented Jun 27, 2024 •

edited

Loading