Skip to content

feat(connector): add connector remote sink - #6493

Merged
mergify[bot] merged 16 commits into
mainfrom
kiv/remote-sink-rw
Nov 28, 2022
Merged

mergify[bot] merged 16 commits into
mainfrom
kiv/remote-sink-rw

Conversation

@KivenChen

@KivenChen KivenChen commented Nov 21, 2022 •

Copy link
Copy Markdown
Contributor

I hereby agree to the terms of the Singularity Data, Inc. Contributor License Agreement.

What's changed and what's your intention?

This PR adds remote sink to connector.

All remote sink operations are passed to the connector node sink service. Specify a connector sink endpoint (currently default to localhost:50051) and a supported sink type to conduct the check.

Checklist

  • I have written necessary rustdoc comments
  • I have added necessary unit tests and integration tests
  • All checks passed in ./risedev check (or alias, ./risedev c)

Documentation

Types of user-facing changes

For instance, query the following to sink changes to a PG table via JDBC

create sink s from t1 with (connector='jdbc', jdbc_url='jdbc:postgresql://localhost:5432/test?user=test&password=connector', table_name='test');

Currently, both RW and the connector node has a support list to check upon. jdbc and file are currently supported.

Release note

Refer to a related PR or issue link (optional)

@github-actions github-actions Bot added the type/feature Type: New feature. label Nov 21, 2022

@wenym1 wenym1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Rest LGTM. Good work!

Comment thread src/common/src/config.rs
Comment thread src/compute/src/lib.rs Outdated
Comment thread src/stream/src/task/env.rs Outdated
Comment thread src/stream/src/executor/sink.rs Outdated
Comment thread src/rpc_client/src/lib.rs Outdated
Comment thread src/connector/src/sink/remote.rs Outdated
Comment thread src/connector/src/sink/remote.rs Outdated
Comment thread src/connector/src/sink/remote.rs Outdated
Comment thread src/connector/src/sink/remote.rs Outdated
Comment thread src/connector/src/sink/remote.rs Outdated
Comment thread src/compute/src/lib.rs
/// Endpoint of the connector node
#[clap(long, default_value = "127.0.0.1:60061")]
pub connector_source_endpoint: String,
#[clap(long, env = "CONNECTOR_SOURCE_ENDPOINT")]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

#6481 is merged, which starts connector node and do e2e testing. After we made this change, we may also have to modify the CI to specify the source endpoint via env var.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We can also add some e2e testing after this PR.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We can also add some e2e testing after this PR.

Maybe the next one. This one is large enough.

Comment thread src/stream/src/task/env.rs Outdated

@wenym1 wenym1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Rest LGTM. Thanks for the great work!

@tabVersion tabVersion left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

generally LGTM

@tabVersion

Copy link
Copy Markdown
Contributor

suggest changing some naming style in with clause

with (
    ...
    jdbc.url='jdbc:postgresql://localhost:5432/test?user=test&password=connector',
    table.name='test'
    ...
)

Comment thread src/stream/src/from_proto/source.rs Outdated
@StrikeW
StrikeW self-requested a review November 24, 2022 10:09

@wenym1 wenym1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Rest LGTM

Comment thread src/source/src/manager.rs
@codecov

codecov Bot commented Nov 25, 2022 •

Copy link
Copy Markdown

Codecov Report

Merging #6493 (d2b27cf) into main (a07f267) will decrease coverage by 0.05%.
The diff coverage is 70.95%.

❗ Current head d2b27cf differs from pull request most recent head 6be4bc2. Consider uploading reports for the commit 6be4bc2 to get more accurate results

@@            Coverage Diff             @@
##             main    #6493      +/-   ##
==========================================
- Coverage   73.86%   73.81%   -0.06%     
==========================================
  Files        1002     1005       +3     
  Lines      162583   163271     +688     
==========================================
+ Hits       120088   120511     +423     
- Misses      42495    42760     +265     
Flag Coverage Δ
rust 73.81% <70.95%> (-0.06%) ⬇️

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

Impacted Files Coverage Δ
src/batch/src/executor/group_top_n.rs 68.42% <ø> (-6.44%) ⬇️
...batch/src/executor/join/distributed_lookup_join.rs 0.00% <0.00%> (ø)
src/batch/src/executor/join/nested_loop_join.rs 91.73% <ø> (ø)
src/batch/src/executor/join/sort_merge_join.rs 79.04% <ø> (ø)
src/batch/src/executor/order_by.rs 95.32% <ø> (ø)
src/batch/src/executor/project_set.rs 76.15% <ø> (ø)
src/batch/src/executor/sys_row_seq_scan.rs 0.00% <0.00%> (ø)
src/batch/src/executor/top_n.rs 75.00% <ø> (ø)
src/batch/src/executor/update.rs 81.36% <ø> (ø)
.../batch/src/task/consistent_hash_shuffle_channel.rs 0.00% <0.00%> (ø)
... and 127 more

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

@KivenChen

Copy link
Copy Markdown
Contributor Author

suggest changing some naming style in with clause

with (
    ...
    jdbc.url='jdbc:postgresql://localhost:5432/test?user=test&password=connector',
    table.name='test'
    ...
)

naming style change now integrated into both sides

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type/feature Type: New feature. user-facing-changes Contains changes that are visible to users

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants