-
Notifications
You must be signed in to change notification settings - Fork 592
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): only store CreateStreamingJob command in tracker #17742
Conversation
Looks like recovery test is failing. Will take a look after it pass. |
…r' into yiming/refactor-create-mv-tracker
The test has passed. It's caused by a bug described in #17613 (comment). @kwannoel PTAL |
@@ -22,13 +22,16 @@ use crate::barrier::Command; | |||
use crate::manager::{ActiveStreamingWorkerNodes, ActorInfos, InflightFragmentInfo, WorkerId}; | |||
use crate::model::{ActorId, FragmentId}; | |||
|
|||
#[derive(Debug, Clone)] |
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.
Can we add some docs for this? Is it for any new fragments associated with the barrier command?
And what is the is_injectable
field for, when is it true/false?
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.
is_injectable
means whether we need to inject the barrier directly to the actors of the fragment. The struct is just extracted from the CommandFragmentChanges::NewFragment
, and no logic is changed in this PR.
These fragments are the most upstream fragments, such as source, now.
}, | ||
&version_stats, | ||
) { | ||
if let Some(command) = tracker.add(&command_ctx, &version_stats) { |
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.
Here
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 #17428, the command finish notifier is removed, and for command other than
CreateStreamingJob
, we simply store it the inCreateMviewTracker
and don't do anything on it. Therefore, in this PR, we change to only store theCreateStreamingJob
command in the tracker.The following refactors are done accordingly or by the way:
CreateStreamingJob
to a separate structCreateStreamingJobCommandInfo
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.