refactor(connector-node): jni reuse bidi stream handle #13131

wenym1 · 2023-10-30T04:58:29Z

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

What's changed and what's your intention?

Currently the jni stream handle of remote sink writer and coordinator is a copy of the original bidi stream handle. In this PR we remove the duplicated code and reuse the original bidi stream.

Besides, we improve the error handling when we call jni methods.

Checklist

I have written necessary rustdoc comments
I have added necessary unit tests and integration tests
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.

gitguardian · 2023-10-30T06:06:03Z

⚠️ GitGuardian has uncovered 2 secrets following the scan of your pull request.

Please consider investigating the findings and remediating the incidents. Failure to do so may lead to compromising the associated services or software components.

🔎 Detected hardcoded secrets in your pull request

GitGuardian id	Secret	Commit	Filename
7648795	Generic CLI Secret	`2bb1fee`	integration_tests/iceberg-cdc/run_test.sh	View secret
7648795	Generic CLI Secret	`2bb1fee`	integration_tests/iceberg-cdc/docker-compose.yml	View secret

🛠 Guidelines to remediate hardcoded secrets

Understand the implications of revoking this secret by investigating where it is used in your code.
Replace and store your secrets safely. Learn here the best practices.
Revoke and rotate these secrets.
If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.

To avoid such incidents in the future consider

following these best practices for managing and storing secrets including API keys and other credentials
install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.

^{🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.

Our GitHub checks need improvements? Share your feedbacks!}

codecov · 2023-10-30T06:31:06Z

Codecov Report

Merging #13131 (7dd40b7) into main (f18e73e) will increase coverage by 0.08%.
Report is 2 commits behind head on main.
The diff coverage is 27.95%.

@@            Coverage Diff             @@
##             main   #13131      +/-   ##
==========================================
+ Coverage   68.23%   68.32%   +0.08%     
==========================================
  Files        1501     1501              
  Lines      253235   253108     -127     
==========================================
+ Hits       172794   172927     +133     
+ Misses      80441    80181     -260

Flag	Coverage Δ
rust	`68.32% <27.95%> (+0.08%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
src/connector/src/lib.rs	`52.08% <ø> (ø)`
src/jni_core/src/macros.rs	`100.00% <ø> (ø)`
src/rpc_client/src/lib.rs	`89.32% <93.93%> (+10.15%)`	⬆️
src/meta/src/manager/sink_coordination/manager.rs	`90.24% <89.47%> (-0.23%)`	⬇️
src/rpc_client/src/sink_coordinate_client.rs	`73.58% <58.33%> (+0.66%)`	⬆️
src/jni_core/src/lib.rs	`1.08% <0.00%> (-0.01%)`	⬇️
src/rpc_client/src/connector_client.rs	`10.04% <0.00%> (+10.04%)`	⬆️
src/connector/src/source/cdc/source/reader.rs	`0.00% <0.00%> (ø)`
src/jni_core/src/jvm_runtime.rs	`0.00% <0.00%> (ø)`
src/connector/src/sink/remote.rs	`35.70% <5.26%> (-2.55%)`	⬇️

... and 16 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

chenzl25 · 2023-10-30T08:00:31Z

src/jni_core/src/jvm_runtime.rs

+    if let Ok(jvm) = JVM.get() {
+        let result: Result<(usize, usize), jni::errors::Error> = try {
+            let mut env = jvm.attach_current_thread()?;
+
+            let runtime_instance = env.call_static_method(


Previously, we used OnceLock here to avoid initializing a JVM when the memory controller collects jvm memory statistics.
#12965 (comment)

Good catch! I have changed to avoid initialize the jvm when collecting metrics.

chenzl25

Rest LGTM. Thanks!

chenzl25 · 2023-10-31T03:45:41Z

src/jni_core/src/jvm_runtime.rs

+    pub fn for_initialized_jvm<O, F: FnOnce(&'static JavaVM) -> O>(&self, f: F) -> Option<O> {
+        JVM_RESULT.get().and_then(|result| {
+            if let Ok(jvm) = result {
+                Some(f(jvm))
+            } else {
+                None
+            }
+        })
    }

-    pub fn get_or_init(&self) -> Result<&JavaVM, &RwError> {
-        self.0.get_or_init(Self::inner_new).as_ref()
+    pub fn get(&self) -> anyhow::Result<&'static JavaVM> {
+        match JVM_RESULT.get_or_init(|| {
+            Self::inner_new().inspect_err(|e| error!("failed to init jvm: {:?}", e))
+        }) {
+            Ok(jvm) => Ok(jvm),
+            Err(e) => Err(anyhow!("jvm not initialized properly: {:?}", e)),
+        }


I think we should follow API semantics of the OnceLock since we use OnceLock as the internal implementation. There are 2 methods get and get_or_init in OnceLock, but in this PR, we use for_initialized_jvm and get corresponding instead which might cause confusion. What about following the same method naming of OnceLock.

Make sense.

I just changed the current get back to get_or_init. The original get will be removed since it is only used by load_jvm_memory_stats which is in the same mod as JvmWrapper and it can use the JVM_RESULT directly.

StrikeW

generally LGTM

StrikeW · 2023-10-31T04:48:41Z

src/rpc_client/src/lib.rs


        let first_response = response_stream
            .next()
            .await
-            .ok_or_else(|| anyhow!("get empty response from start sink request"))??;
+            .ok_or_else(|| anyhow!("get empty response from firstrequest"))??;


refactor(connector-node): jni reuse bidi stream handle

501acad

wenym1 added ci/run-java-binding-tests ci/run-java-connector-node-integration-tests labels Oct 30, 2023

github-actions bot added the type/refactor label Oct 30, 2023

better error handling

a7eb77c

wenym1 added the ci/run-e2e-iceberg-sink-tests label Oct 30, 2023

Merge branch 'main' into yiming/jni-reuse-bidi-stream-handle

2bb1fee

wenym1 force-pushed the yiming/jni-reuse-bidi-stream-handle branch from 6e80b67 to 2bb1fee Compare October 30, 2023 06:05

wenym1 requested review from hzxa21, StrikeW and chenzl25 October 30, 2023 06:49

chenzl25 reviewed Oct 30, 2023

View reviewed changes

do not initialize jvm when collecting metrics

aa7af6b

chenzl25 approved these changes Oct 31, 2023

View reviewed changes

change name to get_or_init

7042a94

wenym1 enabled auto-merge October 31, 2023 05:29

StrikeW approved these changes Oct 31, 2023

View reviewed changes

wenym1 added this pull request to the merge queue Oct 31, 2023

github-merge-queue bot removed this pull request from the merge queue due to a conflict with the base branch Oct 31, 2023

Merge branch 'main' into yiming/jni-reuse-bidi-stream-handle

7dd40b7

wenym1 added this pull request to the merge queue Oct 31, 2023

Merged via the queue into main with commit 59349f1 Oct 31, 2023
5 of 7 checks passed

wenym1 deleted the yiming/jni-reuse-bidi-stream-handle branch October 31, 2023 09:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(connector-node): jni reuse bidi stream handle #13131

refactor(connector-node): jni reuse bidi stream handle #13131

wenym1 commented Oct 30, 2023 •

edited

Loading

gitguardian bot commented Oct 30, 2023 •

edited

Loading

codecov bot commented Oct 30, 2023 •

edited

Loading

chenzl25 Oct 30, 2023

wenym1 Oct 30, 2023

chenzl25 left a comment

chenzl25 Oct 31, 2023

wenym1 Oct 31, 2023

StrikeW left a comment

StrikeW Oct 31, 2023

refactor(connector-node): jni reuse bidi stream handle #13131

refactor(connector-node): jni reuse bidi stream handle #13131

Conversation

wenym1 commented Oct 30, 2023 • edited Loading

What's changed and what's your intention?

Checklist

Documentation

Release note

gitguardian bot commented Oct 30, 2023 • edited Loading

⚠️ GitGuardian has uncovered 2 secrets following the scan of your pull request.

codecov bot commented Oct 30, 2023 • edited Loading

Codecov Report

chenzl25 Oct 30, 2023

Choose a reason for hiding this comment

wenym1 Oct 30, 2023

Choose a reason for hiding this comment

chenzl25 left a comment

Choose a reason for hiding this comment

chenzl25 Oct 31, 2023

Choose a reason for hiding this comment

wenym1 Oct 31, 2023

Choose a reason for hiding this comment

StrikeW left a comment

Choose a reason for hiding this comment

StrikeW Oct 31, 2023

Choose a reason for hiding this comment

wenym1 commented Oct 30, 2023 •

edited

Loading

gitguardian bot commented Oct 30, 2023 •

edited

Loading

codecov bot commented Oct 30, 2023 •

edited

Loading