-
Notifications
You must be signed in to change notification settings - Fork 590
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(meta): build actors in inject barrier #18270
Conversation
AMAZING 🥵 |
let mut map = match job_type { | ||
CreateStreamingJobType::Normal => HashMap::new(), | ||
CreateStreamingJobType::SinkIntoTable(replace_table) => { | ||
replace_table.new_table_fragments.actors_to_create() |
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 job!! Looks like #14904 can be resolved naturally!
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.
scaling part lgtm
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.
LGTM!! It definitely simplify the barrier processing and actor building. 🤔 We need to rethink about #16742 and find another solution.
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.
My only concern is that this may increase the latency for injecting barriers, which could slow down the barrier manager loop. Rest LGTM.
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
After #18104, when handling
BuildActor
request, we simply spawn the actors and save the join handle. After the process of building actors is simplified, we can further postpone spawning actor till inject barrier, so thatbroadcast_actor_info_table
,update_actors
andbuild_actors
can be removed, and the works of these rpc can be done together inInjectBarrier
.drop_actors
while cancelling a command, because for a create streaming job command that is enqueued but not handled yet, its actor is not spawned yet, and there is no need to further drop the actors.In this PR, for simplicity, most changes are on the meta node. The actor locations to be broadcast and the
BuildActorInfo
is included in theInjectBarrierRequest
. On local barrier manager, we still reuse the current implemented methods. The local barrier manager will be refactored in future PRs.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.