-
Notifications
You must be signed in to change notification settings - Fork 572
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
feat: Propagate the CreateMview mutation barrier to init executor epoch #511
Conversation
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.
Generally LGTM.
e06902a
to
0529d75
Compare
Codecov Report
@@ Coverage Diff @@
## main #511 +/- ##
============================================
+ Coverage 72.92% 72.95% +0.03%
Complexity 2683 2683
============================================
Files 876 876
Lines 48900 48994 +94
Branches 1557 1557
============================================
+ Hits 35658 35744 +86
- Misses 12437 12445 +8
Partials 805 805
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
0529d75
to
e7da2dd
Compare
07f50ec
to
c45feb9
Compare
@@ -163,6 +157,16 @@ impl AggExecutor for LocalSimpleAggExecutor { | |||
} | |||
} | |||
|
|||
impl StatefuleExecutor for LocalSimpleAggExecutor { |
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.
LocalSimpleAgg
is stateless now in our system.
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.
Looked at the codes. I would argue that LocalSimpleAgg
is still stateful but the state is not persistent. I think it is okay to keep impl StatefulExecutor
since it enables the initialization check.
} | ||
} | ||
|
||
pub trait StatefuleExecutor: Executor { |
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.
pub trait StatefuleExecutor: Executor { | |
pub trait StatefulExecutor: Executor { |
/// Waiting for the first barrier | ||
INIT, | ||
/// Can read from and write to storage | ||
ACTIVE(u64), |
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.
/// Waiting for the first barrier | |
INIT, | |
/// Can read from and write to storage | |
ACTIVE(u64), | |
/// Waiting for the first barrier | |
Init, | |
/// Can read from and write to storage | |
Active(u64), |
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 catch. Will fix all issues mentioned above in a follow-up PR.
What's changed and what's your intention?
After #399, barrier carriers the next epoch that a stateful executor will use for storage reads/writes. Therefore, executor needs to learn about the init epoch it uses for storage reads and writes before processing stream chunks. This PR addresses this issue by allowing the mutation barrier for mview creation to flow through the mview sub-graph before any data.
Changes include:
Checklist
Refer to a related PR or issue link (optional)