Skip to content

feat(stream): add reset for stream executor #557

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 10 commits into from
Feb 28, 2022
Merged

feat(stream): add reset for stream executor #557

merged 10 commits into from
Feb 28, 2022

Conversation

@yezizp2012
Copy link
Member

@yezizp2012 yezizp2012 commented Feb 25, 2022 •
edited
Loading

What's changed and what's your intention?

First step for introducing rollback barrier.

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)

part of #467

@github-actions github-actions bot added the type/feature Type: New feature. label Feb 25, 2022
@fuyufjh
Copy link
Collaborator

fuyufjh commented Feb 25, 2022

please take a look at #467

@codecov
Copy link

codecov bot commented Feb 25, 2022 •
edited
Loading

Codecov Report

Merging #557 (04c6b4d) into main (084f298) will decrease coverage by 0.06%.
The diff coverage is 5.26%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main     #557      +/-   ##
============================================
- Coverage     72.89%   72.83%   -0.07%     
  Complexity     2683     2683              
============================================
  Files           882      882              
  Lines         49067    49110      +43     
  Branches       1557     1557              
============================================
+ Hits          35769    35770       +1     
- Misses        12493    12535      +42     
  Partials        805      805
Flag Coverage Δ
java 62.54% <ø> (ø)
rust 77.16% <5.26%> (-0.10%) ⬇️

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

Impacted Files Coverage Δ
rust/stream/src/executor/batch_query.rs 75.00% <0.00%> (-1.28%) ⬇️
rust/stream/src/executor/chain.rs 56.36% <0.00%> (-1.05%) ⬇️
rust/stream/src/executor/debug/mod.rs 25.00% <0.00%> (-3.58%) ⬇️
rust/stream/src/executor/filter.rs 77.77% <0.00%> (-1.26%) ⬇️
rust/stream/src/executor/global_simple_agg.rs 65.85% <0.00%> (-2.51%) ⬇️
rust/stream/src/executor/hash_agg.rs 76.97% <0.00%> (-1.70%) ⬇️
rust/stream/src/executor/hash_join.rs 82.81% <0.00%> (-0.65%) ⬇️
rust/stream/src/executor/local_simple_agg.rs 91.20% <0.00%> (-3.02%) ⬇️
...ecutor/managed_state/top_n/top_n_bottom_n_state.rs 79.27% <0.00%> (-0.29%) ⬇️
...am/src/executor/managed_state/top_n/top_n_state.rs 86.19% <0.00%> (-0.33%) ⬇️
... and 11 more

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 084f298...04c6b4d. Read the comment docs.

@yezizp2012
Copy link
Member Author

yezizp2012 commented Feb 25, 2022

please take a look at #467

I agree, and I will work on full recovery firstly. This PR is just a pre work for partial failover.

fuyufjh
fuyufjh approved these changes Feb 28, 2022
View reviewed changes
Copy link
Collaborator

@fuyufjh fuyufjh left a comment

Choose a reason for hiding this comment

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

Generally LGTM

rust/stream/src/executor/mod.rs

fn init(&mut self, _epoch: u64) -> Result<()> {
unreachable!()
}
Copy link
Collaborator

@fuyufjh fuyufjh Feb 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

(unrelated to this PR) @MrCroxx This method was only implemented by BatchQueryExecutor and used by Chain, so perhaps it should not be added to the interface of streaming operators, I think.

BugenZhao
BugenZhao reviewed Feb 28, 2022
View reviewed changes
@yezizp2012 yezizp2012 enabled auto-merge (squash) February 28, 2022 06:45
@fuyufjh fuyufjh mentioned this pull request Feb 28, 2022
Closed
@yezizp2012 yezizp2012 merged commit 9b7cafd into main Feb 28, 2022
@yezizp2012 yezizp2012 deleted the feat/executor-reset branch February 28, 2022 06:59
@xxchan xxchan mentioned this pull request May 6, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fuyufjh fuyufjh fuyufjh approved these changes

@BugenZhao BugenZhao BugenZhao left review comments

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.

5 participants

@yezizp2012 @fuyufjh @BugenZhao @wcy-fdu