Skip to content

feat(watermark): Clean state in DynamicFilter by watermark in right side #6473

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 3 commits into from
Dec 9, 2022
Merged

feat(watermark): Clean state in DynamicFilter by watermark in right side #6473

merged 3 commits into from
Dec 9, 2022

Conversation

soundOfDestiny
Copy link
Contributor

@soundOfDestiny soundOfDestiny commented Nov 20, 2022 •
edited
Loading

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

What's changed and what's your intention?

  • Summarize your change (mandatory)
    Clean state in DynamicFilter by watermark in right side
  • How does this PR work? Need a brief introduction for the changed logic (optional)
    Do range delete by cached watermark among corresponding vnodes on barrier
  • Describe clearly one logical change and avoid lazy messages (optional)
    Buffer and state table will both be cleaned
    BarrierAlign is modified
  • Describe any limitations of the current code (optional)
    If comparator is LessThan, it will not be cleaned

Checklist

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

Refer to a related PR or issue link (optional)

#6472

@github-actions github-actions bot added the type/feature Type: New feature. label Nov 20, 2022
@soundOfDestiny soundOfDestiny changed the title feat(watermark): Clean state in DynamicFilter by watermark feat(watermark): Clean state in DynamicFilter by watermark in right side Nov 20, 2022
@codecov
Copy link

codecov bot commented Nov 20, 2022 •
edited
Loading

Codecov Report

Merging #6473 (86bf660) into main (5ea008d) will increase coverage by 0.01%.
The diff coverage is 94.94%.

@@            Coverage Diff             @@
##             main    #6473      +/-   ##
==========================================
+ Coverage   73.22%   73.24%   +0.01%     
==========================================
  Files        1024     1024              
  Lines      164019   164118      +99     
==========================================
+ Hits       120102   120201      +99     
  Misses      43917    43917
Flag Coverage Δ
rust 73.24% <94.94%> (+0.01%) ⬆️

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

Impacted Files Coverage Δ
src/stream/src/executor/barrier_align.rs 100.00% <ø> (+1.36%) ⬆️
src/stream/src/executor/dynamic_filter.rs 93.42% <ø> (ø)
src/stream/src/executor/hash_join.rs 97.06% <ø> (ø)
...tream/src/executor/managed_state/dynamic_filter.rs 91.01% <94.94%> (+0.96%) ⬆️
...frontend/src/scheduler/hummock_snapshot_manager.rs 58.29% <0.00%> (-0.51%) ⬇️
src/connector/src/source/filesystem/file_common.rs 80.44% <0.00%> (-0.45%) ⬇️
src/common/src/types/ordered_float.rs 32.03% <0.00%> (-0.20%) ⬇️
src/stream/src/executor/aggregation/minput.rs 96.49% <0.00%> (+0.10%) ⬆️
src/stream/src/common/table/state_table.rs 82.96% <0.00%> (+0.37%) ⬆️
... and 1 more

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

Little-Wallace
Little-Wallace reviewed Nov 21, 2022
View reviewed changes
src/stream/src/common/table/state_table.rs Outdated
@@ -691,7 +720,27 @@ impl<S: StateStore> StateTable<S> {
}
}
}
if let Some(range_end_suffix) = range_end_suffix {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it means that we must delete-ranges for every epoch ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does it means that we must delete-ranges for every epoch ?

fixed

Little-Wallace
Little-Wallace reviewed Nov 21, 2022
View reviewed changes
src/stream/src/common/table/state_table.rs Outdated
pub async fn commit(
&mut self,
new_epoch: EpochPair,
watermark: Option<&ScalarImpl>,
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest an independent interface to pass this parameter and update last_watermark.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suggest an independent interface to pass this parameter and update last_watermark.

fixed

@jon-chuang
Copy link
Contributor

jon-chuang commented Nov 21, 2022 •
edited
Loading

Is there a way to add some unit tests based on watermark and check the behaviour? (maybe we can read directly from storage to see if the state below watermark is deleted, and also check the range cache's range is updated).

jon-chuang
jon-chuang reviewed Nov 21, 2022
View reviewed changes
This was referenced Nov 21, 2022
Merged
Merged
@soundOfDestiny
Copy link
Contributor Author

Is there a way to add some unit tests based on watermark and check the behaviour? (maybe we can read directly from storage to see if the state below watermark is deleted, and also check the range cache's range is updated).

I have just added a unit test.

@wcy-fdu wcy-fdu removed their request for review November 23, 2022 05:44
@soundOfDestiny
Copy link
Contributor Author

now there are only dynamic filter changes

@yuhao-su
Copy link
Contributor

yuhao-su commented Dec 8, 2022

Can this be mergerd now?

@soundOfDestiny
Copy link
Contributor Author

soundOfDestiny commented Dec 8, 2022 •
edited
Loading

Can this be mergerd now?

I have no objection. plz kindly ask others which is active in this thread if necessary

jon-chuang
jon-chuang reviewed Dec 9, 2022
View reviewed changes
jon-chuang
jon-chuang approved these changes Dec 9, 2022
View reviewed changes
Copy link
Contributor

@jon-chuang jon-chuang 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

@mergify mergify bot merged commit 672c049 into main Dec 9, 2022
@mergify mergify bot deleted the zl_clndyn branch December 9, 2022 09:14
@soundOfDestiny soundOfDestiny mentioned this pull request Dec 9, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Little-Wallace Little-Wallace Little-Wallace left review comments

@jon-chuang jon-chuang jon-chuang approved these changes

@BugenZhao BugenZhao Awaiting requested review from BugenZhao

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.
4 participants
@soundOfDestiny @jon-chuang @yuhao-su @Little-Wallace