fix(meta): locate upstream source fragment correctly for SourceBackfill #19564
Conversation
xxchan
changed the title
xxchan
force-pushed
the
xxchan/closed-narwhal
branch
2 times, most recently
from
9c3ee4b to
6b449e0
Compare
xxchan
requested review from
shanicky,
fuyufjh,
yezizp2012,
BugenZhao and
st1page
Signed-off-by: xxchan <[email protected]> add test Signed-off-by: xxchan <[email protected]> fix scaling Signed-off-by: xxchan <[email protected]> fix test Signed-off-by: xxchan <[email protected]> fix Signed-off-by: xxchan <[email protected]>
xxchan
force-pushed
the
xxchan/closed-narwhal
branch
from
ad2e783 to
93e7c2b
Compare
xxchan
changed the title
src/prost/src/lib.rs
Outdated
| /// Note: `merge.upstream_actor_id` need to be used with caution. | ||
| /// DO NOT USE if the `StreamNode` is from `Fragment` meta model. | ||
| /// OK to use if the `StreamNode` is from `TableFragments` proto. |
There was a problem hiding this comment.
Why, can you elaborate more? This sounds type-unsafe.
There was a problem hiding this comment.
mentioned here
risingwave/src/meta/model/src/fragment.rs
Lines 29 to 31 in e548a0a
Previously we store TableFragments containing Actors containing (multiple copies of) StreamNode (but the only difference is MergeNode's upstream_actor_id).
Now we just store Fragment containing one StreamNode, and upstream_actor_id is not filled.
There was a problem hiding this comment.
The StreamNode in a Fragment is merely a template. This is likely to be modified or restricted from direct use in the future. All actor relationships should be determined through the upstream_fragment_id in the fragment table, along with the actor and actor dispatcher tables.
| let inner = self.inner.read().await; | ||
| let txn = inner.db.begin().await?; | ||
| let fragment = Fragment::find_by_id(fragment_id) |
There was a problem hiding this comment.
Can we then still use fragment.upstream_fragment_id but check the fragment_type_mask to filter out the desired one?
There was a problem hiding this comment.
It will rely on a new assumption that the upstream fragments can only have 1 Source fragment. It seems safer, but I'd prefer not to rely on it.
Signed-off-by: xxchan <[email protected]>
Signed-off-by: xxchan <[email protected]>
|
@BugenZhao |
src/tests/simulation/tests/integration_tests/scale/shared_source.rs
Outdated
Show resolved
Hide resolved
Signed-off-by: xxchan <[email protected]>
…ll (#19564) Signed-off-by: xxchan <[email protected]>
Signed-off-by: xxchan <[email protected]>
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
fix #19563
Previously, we use
upstream_fragment_idsandupstream_actor_idsin theFragmentandActormodels directly to find the upstream Source of the SourceBackfill.The problem is that we assumed the fragment for SourceBackfill have only one upstream fragment, which is false. There could be other Merge nodes in the fragment, thus other upstream fragments. DynamicFilter is one example where the assumption doesn't hold.
In this PR, we change to use
upstream_fragment_idin the Merge node.Regarding
upstream_actor_id, it's a little troublesome. Seem code and comments for more details.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.