feat(connector): init embedded connector node #12122

chenzl25 · 2023-09-06T07:15:31Z

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

What's changed and what's your intention?

Initialize a JVM to run a embedded connector node in compute nodes and meta nodes.
Environment variable CONNECTOR_LIBS_PATH needs to be specified to tell the JVM where to load connector libs.
After initializing the JVM, we will register native methods for it.
In this PR, only one RPC GetEventStream is replaced with JNI. See more details in the codes.
Related issue Tracking: embedded connector node #12090

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.

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

This reverts commit 8b5b94a.

This reverts commit 25e1a8a.

This reverts commit 018b332.

This reverts commit d9562ff.

StrikeW

Rest LGTM

src/jni_core/src/lib.rs

github-actions

license-eye has totally checked 4080 files.

Valid	Invalid	Ignored	Fixed
1800	1	2279	0

Click to see the invalid file list

java/java-binding/src/main/java/com/risingwave/java/binding/CdcJniChannel.java

java/java-binding/src/main/java/com/risingwave/java/binding/CdcJniChannel.java

…cJniChannel.java Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

chenzl25 · 2023-09-08T09:39:24Z

cc @arkbriar In this PR, we introduce CONNECTOR_LIBS_PATH and JVM_HEAP_SIZE two environment variables.

CONNECTOR_LIBS_PATH needs to be specified to tell the JVM where to load connector libs. IIRC, it should be set to /risingwave/bin/connector-node/libs/ for our cloud development.
JVM_HEAP_SIZE needs to be specified to restrict the max heap size for the JVM. it could be any string or number accepted by the JVM -Xmx argument.

wenym1

Rest LGTM!

...onnector-service/src/main/java/com/risingwave/connector/source/core/JniDbzSourceHandler.java

arkbriar · 2023-09-11T03:49:31Z

cc @arkbriar In this PR, we introduce CONNECTOR_LIBS_PATH and JVM_HEAP_SIZE two environment variables.

CONNECTOR_LIBS_PATH needs to be specified to tell the JVM where to load connector libs. IIRC, it should be set to /risingwave/bin/connector-node/libs/ for our cloud development.

JVM_HEAP_SIZE needs to be specified to restrict the max heap size for the JVM. it could be any string or number accepted by the JVM -Xmx argument.

Sounds great! Two questions:

Will the CONNECTOR_LIBS_PATH be shipped with the image?
Is there any recommended value for JVM_HEAP_SIZE? e.g., is there a minimum requirement?

wenym1

LGTM!

chenzl25 · 2023-09-11T04:14:27Z

Will the CONNECTOR_LIBS_PATH be shipped with the image?

I got what you mean. We can set CONNECTOR_LIBS_PATH to /risingwave/bin/connector-node/libs/ in the dockerfile, so we don't need to set it in the k8s yaml, right?

Is there any recommended value for JVM_HEAP_SIZE? e.g., is there a minimum requirement?

If we don't set it explicitly, it would use 10% of total memory by default. I think a value between 10%-20% looks good to me. As for the minimum requirement, I think 256m is enough.

chenzl25 · 2023-09-11T10:20:32Z

docker/Dockerfile

+# Set default connector libs path
+ENV CONNECTOR_LIBS_PATH /risingwave/bin/connector-node/libs


Add default CONNECTOR_LIBS_PATH to the Dockerfile.

Do I miss any other Dockerfiles? cc @fuyufjh @BugenZhao

AFAIK this is the only one

chenzl25 · 2023-09-11T10:20:32Z

docker/Dockerfile.hdfs

+# Set default connector libs path
+ENV CONNECTOR_LIBS_PATH /risingwave/bin/connector-node/libs


Add default CONNECTOR_LIBS_PATH to the Dockerfile.

IIRC, opandal also uses jni to start a jvm when supporting hdfs access. May need to test whether it will conflict with the embedded connector node. cc @wcy-fdu

wenym1 · 2023-09-12T05:44:00Z

src/connector/src/source/cdc/source/reader.rs

@@ -151,24 +151,33 @@ where

            let channel_ptr = Box::into_raw(tx) as i64;


For current code, we don't even need wrapping the channel with box, since the tx variable is dropped after the method is called. We can pass channel_ptr as &tx as *const Sender as i64.

Co-authored-by: Eric Fu <[email protected]> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

This reverts commit 62901e1.

This reverts commit 4a26072.

chenzl25 and others added 30 commits August 22, 2023 14:41

test jni rs

2904555

CHANNEL POOL

2831bad

support jni create cdc source

8e3fc67

fmt

bea170f

fmt

768776d

fmt

03664e9

remove cdylib

37e530e

fmt

89d421f

use never inline for run_this_func_to_get_valid_ptr_from_java_binding

61f5463

refine sendMsgToChannel

1f2d3ae

add copyright

2e3c93f

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

support resouce reclamation

41583db

drop channel pointer properly

3041df2

resolve conflicts

2dbbb51

serialize proto msg in jni

ef0f0c8

refactor

a940f8d

first version of embedded connector node

d0ef67f

refine

df9fc72

better naming

071b96d

resolve conflict

738a2dd

remove jvm related codes

d9562ff

remove bin in jni_core

4f4bb7f

refine cargo toml

018b332

add necessary dependencies to java_binding

25e1a8a

fmt

8b5b94a

Revert "fmt"

41bd111

This reverts commit 8b5b94a.

Revert "add necessary dependencies to java_binding"

4b2089f

This reverts commit 25e1a8a.

Revert "refine cargo toml"

35ab88a

This reverts commit 018b332.

Revert "remove jvm related codes"

8bcce49

This reverts commit d9562ff.

fmt

e16d040

add cdcJniChannelClose

67f0e72

StrikeW approved these changes Sep 8, 2023

View reviewed changes

src/jni_core/src/lib.rs Show resolved Hide resolved

github-actions bot reviewed Sep 8, 2023

View reviewed changes

java/java-binding/src/main/java/com/risingwave/java/binding/CdcJniChannel.java Outdated Show resolved Hide resolved

chenzl25 requested a review from wenym1 September 8, 2023 09:30

Update java/java-binding/src/main/java/com/risingwave/java/binding/Cd…

cfa8ac6

…cJniChannel.java Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

resolve conflicts

0d87139

wenym1 reviewed Sep 11, 2023

View reviewed changes

...onnector-service/src/main/java/com/risingwave/connector/source/core/JniDbzSourceHandler.java Show resolved Hide resolved

refine

366eba9

wenym1 approved these changes Sep 11, 2023

View reviewed changes

chenzl25 added this pull request to the merge queue Sep 11, 2023

chenzl25 removed this pull request from the merge queue due to a manual request Sep 11, 2023

chenzl25 and others added 2 commits September 11, 2023 17:38

drop channel pointer in rust side

dd0142b

Merge branch 'main' into dylan/init_embedded_connector_node

c184ade

chenzl25 commented Sep 11, 2023

View reviewed changes

Merge branch 'main' into dylan/init_embedded_connector_node

7208816

wenym1 reviewed Sep 12, 2023

View reviewed changes

refine

e6fbfde

chenzl25 enabled auto-merge September 12, 2023 08:01

Merge branch 'main' into dylan/init_embedded_connector_node

ba14875

chenzl25 added this pull request to the merge queue Sep 12, 2023

Merged via the queue into main with commit 62901e1 Sep 12, 2023
7 of 8 checks passed

chenzl25 deleted the dylan/init_embedded_connector_node branch September 12, 2023 10:55

Li0k pushed a commit that referenced this pull request Sep 15, 2023

feat(connector): init embedded connector node (#12122)

fb0622d

Co-authored-by: Eric Fu <[email protected]> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

StrikeW added a commit that referenced this pull request Sep 25, 2023

Revert "feat(connector): init embedded connector node (#12122)"

4a26072

This reverts commit 62901e1.

StrikeW added a commit that referenced this pull request Oct 6, 2023

Revert "Revert "feat(connector): init embedded connector node (#12122)""

3c7fd1b

This reverts commit 4a26072.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(connector): init embedded connector node #12122

feat(connector): init embedded connector node #12122

chenzl25 commented Sep 6, 2023 •

edited

Loading

StrikeW left a comment

github-actions bot left a comment

chenzl25 commented Sep 8, 2023 •

edited

Loading

wenym1 left a comment

arkbriar commented Sep 11, 2023 •

edited by chenzl25

Loading

wenym1 left a comment

chenzl25 commented Sep 11, 2023

chenzl25 Sep 11, 2023

chenzl25 Sep 11, 2023

fuyufjh Sep 11, 2023

chenzl25 Sep 11, 2023

wenym1 Sep 12, 2023

wenym1 Sep 12, 2023

		# Set default connector libs path
		ENV CONNECTOR_LIBS_PATH /risingwave/bin/connector-node/libs

		@@ -151,24 +151,33 @@ where

		let channel_ptr = Box::into_raw(tx) as i64;

feat(connector): init embedded connector node #12122

feat(connector): init embedded connector node #12122

Conversation

chenzl25 commented Sep 6, 2023 • edited Loading

What's changed and what's your intention?

Checklist

Documentation

Release note

StrikeW left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

chenzl25 commented Sep 8, 2023 • edited Loading

wenym1 left a comment

Choose a reason for hiding this comment

arkbriar commented Sep 11, 2023 • edited by chenzl25 Loading

wenym1 left a comment

Choose a reason for hiding this comment

chenzl25 commented Sep 11, 2023

chenzl25 Sep 11, 2023

Choose a reason for hiding this comment

chenzl25 Sep 11, 2023

Choose a reason for hiding this comment

fuyufjh Sep 11, 2023

Choose a reason for hiding this comment

chenzl25 Sep 11, 2023

Choose a reason for hiding this comment

wenym1 Sep 12, 2023

Choose a reason for hiding this comment

wenym1 Sep 12, 2023

Choose a reason for hiding this comment

chenzl25 commented Sep 6, 2023 •

edited

Loading

chenzl25 commented Sep 8, 2023 •

edited

Loading

arkbriar commented Sep 11, 2023 •

edited by chenzl25

Loading