feat(meta): support ddl progress - #7914
Conversation
Codecov Report
@@ Coverage Diff @@
## main #7914 +/- ##
==========================================
- Coverage 71.61% 71.56% -0.06%
==========================================
Files 1116 1116
Lines 179648 179819 +171
==========================================
+ Hits 128660 128686 +26
- Misses 50988 51133 +145
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
Will review it later. 🥰 |
|
sink executor also relies on the chain executor, can it help to see the progress of creating sink? |
Yes, it can show progress of creating table, mv, index and sink. |
| let consumed_rows = match self.state { | ||
| Some(ChainState::ConsumingUpstream(last, consumed_row)) => { | ||
| assert!(last < consumed_epoch); | ||
| consumed_row + rows |
There was a problem hiding this comment.
Seems the rows is an accumulated value, and there's no need to sum them up here? 👀
There was a problem hiding this comment.
For backfill executor, it is not an accumulated value, but make it an accumulated value seems clearer. Let's change it.
| } | ||
| ChainState::Done => panic!("should not report done multiple times"), | ||
| } | ||
| self.calculate_progress(); |
There was a problem hiding this comment.
What about only calculating in-place when gen_ddl_progress, so that we don't need to maintain one more field?
There was a problem hiding this comment.
Actually I use it as a state to ensure the progress value will never go down, because the calculation itself doesn't guarantee the monotonicity.
| ConsumingSnapshot, | ||
| ConsumingUpstream(Epoch), | ||
| ConsumingUpstream(Epoch, ConsumedRows), | ||
| Done, |
There was a problem hiding this comment.
This is originally designed for the rearranged-chain, and it seems not much consistent with the current backfill logic. For example, ConsumingSnapshot is never used. We can clean it up in the future if unnecessary. 😄
| /// DDL definition. | ||
| pub definition: String, |
There was a problem hiding this comment.
I suggest we directly put the stream_job in the context, so that definition and table_properties can be covered. Let's refactor this in the future.
| * version_stats | ||
| .table_stats | ||
| .get(&upstream_mv.table_id) | ||
| .map_or(0, |stat| stat.total_key_count as u64) |
There was a problem hiding this comment.
Hummock's stats are for the total keys, including the historical ones. Assuming that there're a lot of Updates in the upstream mview, if the rows come from the "upstream", we'll count them correctly, but for those from "snapshot", a single consumed row may correspond to multiple historical rows in the stats. Not sure whether this impacts a lot.
There was a problem hiding this comment.
Yes, our progress calculation is an approximate value instead of accurate value. It is hard to calculate the accurate value based on what we have currently. From the user perspective, I think they will care about the DDL will take a long time that is the snapshot contains a large amount of data. In this case, I hope with the help of compaction, we can guarantee the total key count will not exceed (for example) 30% of the actual key count. BTW, if total key count is larger than the actual key count, our calculated progress number is smaller than the actual progress and this is acceptable as long as we provide a lower bound value.
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
rw_catalog.rw_ddl_progressto show ddl progress.rw_ddl_progresswill fetch active ddl progress from meta by the rpc methodget_ddl_progress.Example
Checklist
./risedev check(or alias,./risedev c)Documentation
Click here for Documentation
Types of user-facing changes
Please keep the types that apply to your changes, and remove the others.
Release note
rw_catalog.rw_ddl_progressto show ddl progress.closes #7418