refactor: remove create_source/sync_source rpc in CN(#5269) #5654

PanQL · 2022-09-30T03:37:00Z

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

What's changed and what's your intention?

we wrap source info into SourceNode, so that SourceExecutorBuilder can build stream source without local source manager. By this way, we can avoid calling create_souce rpc of CNs when creating a new stream source.

Add SourceDescBuilder to avoid using local source manager in SouceExecutor and SourceExecutorBuilder
compute: remove create_source/sync_source rpc
meta: avoid sending create_source request to CN when doing create_source
meta: avoid sending sync_sources request to CN when doing recovery
source: remove create_source/create_table_source in SourceManager
fix unit-tests

Please explain IN DETAIL what the changes are in this PR and why they are needed:

Summarize your change (mandatory)
How does this PR work? Need a brief introduction for the changed logic (optional)
Describe clearly one logical change and avoid lazy messages (optional)
Describe any limitations of the current code (optional)

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

If your pull request contains user-facing changes, please specify the types of the changes, and create a release note. Otherwise, please feel free to remove this section.

Types of user-facing changes

Please keep the types that apply to your changes, and remove those that do not apply.

Installation and deployment
Connector (sources & sinks)
SQL commands, functions, and operators
RisingWave cluster configuration changes
Other (please specify in the release note below)

Release note

Please create a release note for your changes. In the release note, focus on the impact on users, and mention the environment or conditions where the impact may occur.

Refer to a related PR or issue link (optional)

#5269

CLAassistant · 2022-09-30T03:37:04Z

All committers have signed the CLA.

codecov · 2022-09-30T04:23:44Z

Codecov Report

Merging #5654 (1ae7117) into main (ed607ac) will decrease coverage by 0.02%.
The diff coverage is 80.80%.

@@            Coverage Diff             @@
##             main    #5654      +/-   ##
==========================================
- Coverage   74.31%   74.29%   -0.03%     
==========================================
  Files         923      924       +1     
  Lines      144284   144308      +24     
==========================================
- Hits       107228   107211      -17     
- Misses      37056    37097      +41

Flag	Coverage Δ
rust	`74.29% <80.80%> (-0.03%)`	⬇️

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

Impacted Files	Coverage Δ
src/common/src/error.rs	`72.30% <ø> (ø)`
src/common/src/util/prost.rs	`0.00% <0.00%> (ø)`
src/compute/src/rpc/service/stream_service.rs	`0.00% <ø> (ø)`
src/frontend/src/catalog/root_catalog.rs	`81.81% <0.00%> (+0.32%)`	⬆️
src/meta/src/barrier/recovery.rs	`66.96% <ø> (-2.20%)`	⬇️
src/meta/src/stream/scale.rs	`16.42% <0.00%> (ø)`
src/meta/src/stream/stream_manager.rs	`71.20% <ø> (+0.27%)`	⬆️
src/rpc_client/src/stream_client.rs	`100.00% <ø> (ø)`
src/source/src/lib.rs	`93.33% <ø> (ø)`
src/storage/src/error.rs	`0.00% <ø> (ø)`
... and 64 more

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

shanicky

StreamSource part LGTM, I tested it with kafka source and it works fine, including the addition of the new kafka partition.

yezizp2012 · 2022-09-30T07:41:35Z

@PanQL please sign the SLA.

yezizp2012

LGTM

shanicky · 2022-09-30T07:58:19Z

After a simple test, after merging this PR with #5648, we can migrate/scale the StreamSourceActor to the newly joined node at will. cc @yezizp2012 @KeXiangWang

BugenZhao

Good work!

src/frontend/src/catalog/source_catalog.rs

src/stream/src/executor/source/source_executor.rs

src/meta/src/stream/source_manager.rs

BugenZhao · 2022-09-30T15:05:29Z

Please resolve the comments before getting it merged. 🥰

src/common/src/util/prost.rs

src/frontend/src/catalog/source_catalog.rs

src/stream/src/executor/source/source_executor.rs

BugenZhao

LGTM. Good work! 🚀

src/stream/src/executor/source/source_executor.rs

BugenZhao · 2022-10-06T10:38:46Z

src/source/src/manager.rs

+        let mut sources = mgr.get_sources()?;
+        if let Some(source_desc) = sources.get(table_id) {
+            return Ok(source_desc.clone());
+        }


I think it would be better to move this single-flight logic to the TableSourceManager (or something else) itself to avoid exposing the inner structure. Besides, the trait and the Mem impl can also be cleaned up.

Let's do this in future PRs!

…m source. Signed-off-by: Qinglin Pan <[email protected]>

Signed-off-by: Qinglin Pan <[email protected]>

…urce rpc - move create_table_source logic into SourceDescBuilder - remove create_source rpc in CN - remove sync_source rpc in CN - fix all unittests' complaination Signed-off-by: Qinglin Pan <[email protected]>

Signed-off-by: Qinglin Pan <[email protected]>

Co-authored-by: Bugen Zhao <[email protected]> Signed-off-by: Qinglin Pan <[email protected]>

shanicky

LGTM

PanQL requested review from yezizp2012 and BugenZhao September 30, 2022 03:37

github-actions bot added the type/refactor label Sep 30, 2022

PanQL force-pushed the panpan/refactor-local-source-mgr branch from 97041eb to 3bebd35 Compare September 30, 2022 03:41

yezizp2012 requested a review from shanicky September 30, 2022 03:45

shanicky requested a review from tabVersion September 30, 2022 07:21

shanicky reviewed Sep 30, 2022

View reviewed changes

yezizp2012 approved these changes Sep 30, 2022

View reviewed changes

PanQL added the mergify/can-merge label Sep 30, 2022

PanQL assigned shanicky Sep 30, 2022

shanicky assigned PanQL and unassigned shanicky Sep 30, 2022

BugenZhao reviewed Sep 30, 2022

View reviewed changes

BugenZhao removed the mergify/can-merge label Sep 30, 2022

PanQL force-pushed the panpan/refactor-local-source-mgr branch from dcd4c76 to 7193886 Compare October 5, 2022 16:37

PanQL changed the title ~~refactor: remove stream source related part of local source manager(#5269)~~ refactor: remove create_source/sync_source rpc in CN(#5269) Oct 6, 2022

BugenZhao reviewed Oct 6, 2022

View reviewed changes

PanQL force-pushed the panpan/refactor-local-source-mgr branch 2 times, most recently from abf81f8 to c3b6474 Compare October 6, 2022 08:09

BugenZhao approved these changes Oct 6, 2022

View reviewed changes

BugenZhao requested review from shanicky and yezizp2012 October 6, 2022 10:39

Qinglin Pan added 3 commits October 6, 2022 19:34

refactor: wrap source creation into SourceDescBuilder to create strea…

c4ca6ac

…m source. Signed-off-by: Qinglin Pan <[email protected]>

remove StreamSource related part of create_source rpc in CN

fb7ae3d

Signed-off-by: Qinglin Pan <[email protected]>

fix typo

f911e92

Signed-off-by: Qinglin Pan <[email protected]>

Qinglin Pan added 8 commits October 6, 2022 19:34

fix unit test

58d2861

Signed-off-by: Qinglin Pan <[email protected]>

remove SourceType field in SourceNode

1e5ba1c

Signed-off-by: Qinglin Pan <[email protected]>

remap row_id_index in SourceExecutor

a021c12

Signed-off-by: Qinglin Pan <[email protected]>

build table source in SourceDescBuilder, remove create_source/sync_so…

40c427a

…urce rpc - move create_table_source logic into SourceDescBuilder - remove create_source rpc in CN - remove sync_source rpc in CN - fix all unittests' complaination Signed-off-by: Qinglin Pan <[email protected]>

fix misc check error

ec0b0ed

Signed-off-by: Qinglin Pan <[email protected]>

move create_table_info into source::table::test_utils

82c26d9

Signed-off-by: Qinglin Pan <[email protected]>

remove row_id_index field from source_executor

9d09cc7

Signed-off-by: Qinglin Pan <[email protected]>

remove SourceCatalogType in frontend

56db28b

Signed-off-by: Qinglin Pan <[email protected]>

PanQL force-pushed the panpan/refactor-local-source-mgr branch from c3b6474 to 56db28b Compare October 6, 2022 11:34

Update src/stream/src/executor/source/source_executor.rs

1ae7117

Co-authored-by: Bugen Zhao <[email protected]> Signed-off-by: Qinglin Pan <[email protected]>

PanQL force-pushed the panpan/refactor-local-source-mgr branch from e514e81 to 1ae7117 Compare October 6, 2022 11:45

shanicky added the mergify/can-merge label Oct 6, 2022

mergify bot merged commit bd3bd59 into main Oct 6, 2022

mergify bot deleted the panpan/refactor-local-source-mgr branch October 6, 2022 14:18

shanicky approved these changes Oct 6, 2022

View reviewed changes

BugenZhao mentioned this pull request Oct 18, 2022

bug: empty source manager in newly joined compute nodes #5269

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: remove create_source/sync_source rpc in CN(#5269) #5654

refactor: remove create_source/sync_source rpc in CN(#5269) #5654

PanQL commented Sep 30, 2022 •

edited

Loading

CLAassistant commented Sep 30, 2022 •

edited

Loading

codecov bot commented Sep 30, 2022 •

edited

Loading

shanicky left a comment

yezizp2012 commented Sep 30, 2022

yezizp2012 left a comment

shanicky commented Sep 30, 2022

BugenZhao left a comment

BugenZhao commented Sep 30, 2022

BugenZhao left a comment

BugenZhao Oct 6, 2022

shanicky left a comment

refactor: remove create_source/sync_source rpc in CN(#5269) #5654

refactor: remove create_source/sync_source rpc in CN(#5269) #5654

Conversation

PanQL commented Sep 30, 2022 • edited Loading

What's changed and what's your intention?

Checklist

Documentation

Types of user-facing changes

Release note

Refer to a related PR or issue link (optional)

CLAassistant commented Sep 30, 2022 • edited Loading

codecov bot commented Sep 30, 2022 • edited Loading

Codecov Report

shanicky left a comment

Choose a reason for hiding this comment

yezizp2012 commented Sep 30, 2022

yezizp2012 left a comment

Choose a reason for hiding this comment

shanicky commented Sep 30, 2022

BugenZhao left a comment

Choose a reason for hiding this comment

BugenZhao commented Sep 30, 2022

BugenZhao left a comment

Choose a reason for hiding this comment

BugenZhao Oct 6, 2022

Choose a reason for hiding this comment

shanicky left a comment

Choose a reason for hiding this comment

PanQL commented Sep 30, 2022 •

edited

Loading

CLAassistant commented Sep 30, 2022 •

edited

Loading

codecov bot commented Sep 30, 2022 •

edited

Loading