Skip to content

feat: drop mv on mv #506

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Feb 23, 2022
Merged

feat: drop mv on mv #506

merged 7 commits into from
Feb 23, 2022

Conversation

@BugenZhao
Copy link
Member

@BugenZhao BugenZhao commented Feb 23, 2022 •
edited
Loading

What's changed and what's your intention?

This PR implements dropping mv on mv by Stop mutation. Thanks to the reference counting for materialized view in #364, we are safe to stop and drop actors when asked.

The step of dropping mv on mv is:

  1. Meta: get the fragments by table (mv) id.
  2. Meta: send a barrier carrying Stop mutation with these actor ids. Note that we are still to collect barriers from these "dropping" actors.
  3. Compute: dispatch barriers to all outputs, THEN remove outputs in dispatcher if needed.
  4. Compute: stop actors by breaking the loop if asked.
  5. Meta: collect the barriers, then call drop_actors to remove all actor info.
  6. Meta: remove info from table fragments store.

Checklist

  • I have written necessary docs and comments
  • I have added necessary unit tests and integration tests

Refer to a related PR or issue link (optional)

Close #412.

@codecov
Copy link

codecov bot commented Feb 23, 2022 •
edited
Loading

Codecov Report

Merging #506 (9914fe3) into main (2515c73) will decrease coverage by 0.01%.
The diff coverage is 48.48%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main     #506      +/-   ##
============================================
- Coverage     72.65%   72.64%   -0.02%     
  Complexity     2709     2709              
============================================
  Files           875      875              
  Lines         48855    48874      +19     
  Branches       1556     1557       +1     
============================================
+ Hits          35498    35506       +8     
- Misses        12547    12557      +10     
- Partials        810      811       +1
Flag Coverage Δ
java 63.40% <0.00%> (-0.01%) ⬇️
rust 76.56% <51.61%> (-0.02%) ⬇️

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

Impacted Files Coverage Δ
...risingwave/execution/handler/DropTableHandler.java 85.18% <0.00%> (-3.28%) ⬇️
rust/common/src/error.rs 67.88% <0.00%> (ø)
rust/meta/src/barrier/command.rs 38.00% <0.00%> (+0.74%) ⬆️
rust/meta/src/rpc/service/stream_service.rs 100.00% <ø> (ø)
rust/stream/src/task/stream_manager.rs 39.19% <0.00%> (-0.09%) ⬇️
rust/stream/src/executor/dispatch.rs 79.34% <54.16%> (-1.71%) ⬇️
rust/stream/src/executor/batch_query.rs 76.27% <100.00%> (ø)
rust/stream/src/executor/chain.rs 83.33% <100.00%> (ø)
rust/stream/src/executor/barrier_align.rs 78.78% <0.00%> (ø)
rust/common/src/types/ordered_float.rs 25.82% <0.00%> (+0.33%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2515c73...9914fe3. Read the comment docs.

@BugenZhao
refine docs
11b765d 
Signed-off-by: Bugen Zhao <[email protected]>
@BugenZhao
drop mv in mv_on_mv.slt
166ba0a 
Signed-off-by: Bugen Zhao <[email protected]>
MrCroxx
MrCroxx approved these changes Feb 23, 2022
View reviewed changes
Copy link
Contributor

@MrCroxx MrCroxx left a comment

Choose a reason for hiding this comment

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

Rest LGTM, good job!

@BugenZhao BugenZhao marked this pull request as ready for review February 23, 2022 11:39
@BugenZhao BugenZhao changed the title [WIP] feat: drop mv on mv feat: drop mv on mv Feb 23, 2022
@github-actions github-actions bot added the type/feature Type: New feature. label Feb 23, 2022
yezizp2012
yezizp2012 approved these changes Feb 23, 2022
View reviewed changes
Copy link
Member

@yezizp2012 yezizp2012 left a comment

Choose a reason for hiding this comment

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

Good job, LGTM!!!!

lmatz
lmatz approved these changes Feb 23, 2022
View reviewed changes
Copy link
Contributor

@lmatz lmatz left a comment

Choose a reason for hiding this comment

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

success

@BugenZhao BugenZhao merged commit 93e53f8 into main Feb 23, 2022
@BugenZhao BugenZhao deleted the bz/drop-mv-on-mv branch February 23, 2022 12:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lmatz lmatz lmatz approved these changes

@yezizp2012 yezizp2012 yezizp2012 approved these changes

@MrCroxx MrCroxx Awaiting requested review from MrCroxx

@wyhyhyhyh wyhyhyhyh Awaiting requested review from wyhyhyhyh

Assignees

No one assigned

Labels

type/feature Type: New feature.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

streaming: fix drop mv and drop mv on mv

5 participants

@BugenZhao @lmatz @yezizp2012 @MrCroxx