refactor: ensure update vnode bitmap after yield barrier #20218
+531
−283
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This pull request has been modified. If you want me to regenerate unit test for any of the files related, please find the file in "Files Changed" tab and add a comment |
wenym1
force-pushed
the
yiming/state-table-post-yield-update-vnode
branch
from
7ebc0d1 to
4083a47
Compare
wenym1
changed the title
wenym1
commented
Feb 6, 2025
| .as_mut() | ||
| .unwrap() | ||
| .commit(barrier.epoch) | ||
| .await?; | ||
| yield Message::Barrier(barrier); | ||
| post_commit |
There was a problem hiding this comment.
Newly added logic. Previously the vnode bitmap of memo table is not updated, not sure if this is correct or not.
| if previous_vnode_bitmap != vnode_bitmap { | ||
| need_update_global_max_watermark = true; | ||
| } | ||
| } | ||
|
|
||
| if is_checkpoint && last_checkpoint_watermark != current_watermark { |
There was a problem hiding this comment.
Logic is changed here. Previously write the watermark on the new vnode bitmap. After this PR will write on the original vnode bitmap.
|
|
||
| Message::Barrier(b) | ||
| continue; |
There was a problem hiding this comment.
The barrier has been yielded, so we change to continue here.
wenym1
force-pushed
the
yiming/state-table-post-yield-update-vnode
branch
from
0d278ab to
9965471
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
Part of #18312.
As described in #18312, we should ensure a correct handling order as
state_table.commit()->yield barrier->state_table.update_vnode_bitmap()in streaming executors while update vnode bitmap.In this PR, for
state_table.commit(), we change to return aStateTablePostCommit<'_>, which captures the mutable reference to the originalstate_tableand will be used as a callback after yielding the barrier. TheStateTablePostCommithas a methodasync fn post_yield_barrier(mut self, new_vnodes: Option<Arc<Bitmap>>)to be called after the we yield the barrier.The
StateTablePostCommitis marked with#[must_use]. The method namepost_yield_barrierindicates that it should be called after we have yielded the barrier. In state table, we add a flagon_post_commit, to indicate that whether theStateTablePostCommitis handled properly. Onstate_table.commit(), we will mark theon_post_commitas true, and inStateTablePostCommit::post_yield_barrier, we will remark the flag as false, and onstate_table.commit(), we will assert that theon_post_commitmust be false. Note that, thepost_yield_barriershould be called for all barriers rather than only for the barrier with update vnode bitmap. In this way, though we don't have scale test for all streaming executor, we can ensure that all executor covered by normal e2e test have properly handled theStateTablePostCommit.Checklist
Documentation
Release note