Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(source): merge all inner reader into one stream #5611

Merged
merged 4 commits into from
Sep 28, 2022

Conversation

waruto210
Copy link
Contributor

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

What's changed and what's your intention?

PLEASE DO NOT LEAVE THIS EMPTY !!!

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

Merge all streams of InnerConnectorSourceReader into one stream to avoid spawn too many tasks.

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)

Signed-off-by: waruto <wmc314@outlook.com>
@CLAassistant
Copy link

CLAassistant commented Sep 28, 2022

CLA assistant check
All committers have signed the CLA.

@waruto210 waruto210 changed the title merge all inner reader into one stream refactor(source) merge all inner reader into one stream Sep 28, 2022
@waruto210 waruto210 changed the title refactor(source) merge all inner reader into one stream refactor(source): merge all inner reader into one stream Sep 28, 2022
@BugenZhao BugenZhao self-requested a review September 28, 2022 07:54
@waruto210 waruto210 marked this pull request as ready for review September 28, 2022 07:56
@codecov
Copy link

codecov bot commented Sep 28, 2022

Codecov Report

Merging #5611 (9624f4c) into main (c9dc5b9) will decrease coverage by 0.07%.
The diff coverage is 66.76%.

@@            Coverage Diff             @@
##             main    #5611      +/-   ##
==========================================
- Coverage   74.31%   74.24%   -0.08%     
==========================================
  Files         907      915       +8     
  Lines      142996   143258     +262     
==========================================
+ Hits       106269   106362      +93     
- Misses      36727    36896     +169     
Flag Coverage Δ
rust 74.24% <66.76%> (-0.08%) ⬇️

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

Impacted Files Coverage Δ
src/batch/src/executor/row_seq_scan.rs 17.59% <ø> (ø)
src/common/src/collection/evictable.rs 64.70% <ø> (-7.88%) ⬇️
src/common/src/config.rs 53.24% <0.00%> (-1.79%) ⬇️
...c/compute/src/compute_observer/observer_manager.rs 64.70% <ø> (ø)
src/compute/src/lib.rs 2.77% <ø> (ø)
src/compute/src/rpc/service/stream_service.rs 0.00% <0.00%> (ø)
src/compute/src/server.rs 0.00% <0.00%> (ø)
src/ctl/src/cmd_impl/hummock/observer.rs 0.00% <ø> (ø)
src/meta/src/hummock/error.rs 19.04% <ø> (ø)
src/meta/src/hummock/manager/compaction.rs 99.31% <ø> (-0.05%) ⬇️
... and 79 more

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

src/source/Cargo.toml Outdated Show resolved Hide resolved
src/source/src/connector_source.rs Outdated Show resolved Hide resolved
src/source/src/connector_source.rs Outdated Show resolved Hide resolved
message_tx: Sender<Result<Vec<SourceMessage>>>,

// merge all streams of inner reader into one
all_reader_stream: BoxStream<'static, Result<Vec<SourceMessage>>>,
Copy link
Member

Choose a reason for hiding this comment

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

How about using type_alias_impl_trait to avoid this Box<dyn>?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That causes build errors.

image

Copy link
Member

Choose a reason for hiding this comment

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

Defining the type alias in a submodule may help this. Anyway, since we yield chunks instead of rows, this won't hurt much.

Copy link
Contributor Author

@waruto210 waruto210 Sep 28, 2022

Choose a reason for hiding this comment

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

Defining the type alias in a submodule may help this. Anyway, since we yield chunks instead of rows, this won't hurt much.

Still an error, I comment it with todo, maybe after this feature is stable.

Copy link
Contributor

@tabVersion tabVersion left a comment

Choose a reason for hiding this comment

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

basically LGTM

@mergify mergify bot merged commit fea0bf9 into risingwavelabs:main Sep 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants