-
Notifications
You must be signed in to change notification settings - Fork 569
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: add target_offsets to determinine if source backfill finished #18297
refactor: add target_offsets to determinine if source backfill finished #18297
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
e9f69ef
to
27cf1fc
Compare
@@ -727,6 +766,18 @@ impl<S: StateStore> SourceBackfillExecutorInner<S> { | |||
debug_assert_eq!(old_states, target_state); | |||
} | |||
stage.states = target_state; | |||
stage.splits = target_splits; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line fixed a bug https://github.com/risingwavelabs/risingwave/pull/18296/files#r1738292017
27cf1fc
to
aa5610d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good
let old_target_offsets = std::mem::take(&mut stage.target_offsets); | ||
stage.target_offsets = stage | ||
.states | ||
.keys() | ||
.map(|split_id| { | ||
( | ||
split_id.clone(), | ||
old_target_offsets.get(split_id).cloned().flatten(), | ||
) | ||
}) | ||
.collect(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HashMap::retain
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to consider newly added splits.
aa5610d
to
272a7a4
Compare
c8b80b7
to
2b8f062
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great!
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
step 1 of #18299, see there for the motivation and a state diagram of the new design.
The main idea of the code change is to consolidate logic into
handle_upstream_row
andhandle_backfill_row
.Checklist
./risedev check
(or alias,./risedev c
)Documentation
Release note
If this PR includes changes that directly affect users or other significant modifications relevant to the community, kindly draft a release note to provide a concise summary of these changes. Please prioritize highlighting the impact these changes will have on users.