feat: persist processed rows for SourceBackfill for SHOW JOBS #18983

xxchan · 2024-10-17T15:42:44Z

I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.

What's changed and what's your intention?

Follow up of #18925

If we don't persist, after recovery the progress will start from 0 again.

However, TBH I'm a little hesitant about this change, since it is a little ugly. And we have to persist it just for SHOW JOBS.

This changes the state table representation, and should be a breaking change. But if we can get this into 2.1, then we don't need to consider compatibility.

Besides SHOW JOBS, this also benefits scanning from state table, making it more informative. (See diff on slt)

Checklist

I have written necessary rustdoc comments
I have added necessary unit tests and integration tests
I have added test labels as necessary. See details.
I have added fuzzing tests or opened an issue to track them. (Optional, recommended for new SQL features Sqlsmith: Sql feature generation #7934).
My PR contains breaking changes. (If it deprecates some features, please create a tracking issue to remove them in the future).
All checks passed in ./risedev check (or alias, ./risedev c)
My PR changes performance-critical code. (Please run macro/micro-benchmarks and show the results.)

My PR contains critical fixes that are necessary to be merged into the latest release. (Please check out the details)

Documentation

My PR needs documentation updates. (Please use the Release note section below to summarize the impact on users)

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.

xxchan · 2024-10-17T15:43:01Z

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @xxchan and the rest of your teammates on Graphite

xxchan · 2024-10-17T15:46:50Z

src/stream/src/executor/source/source_backfill_executor.rs

+impl BackfillStateWithCnt {
    pub fn encode_to_json(self) -> JsonbVal {
        serde_json::to_value(self).unwrap().into()
    }


This changes the state table representation, and should be a breaking change. But if we can get this into 2.1, then we don't need to consider compatibility.

xxchan · 2024-10-17T15:47:49Z

src/stream/src/executor/source/source_backfill_executor.rs

@@ -668,23 +682,15 @@ impl<S: StateStore> SourceBackfillExecutorInner<S> {
                            let new_chunk = chunk.clone_with_vis(new_vis);
                            yield Message::Chunk(new_chunk);
                            source_backfill_row_count.inc_by(card as u64);
-                            total_backfilled_rows += card as u64;


Previously, we just need to += card here for total cnt

xxchan · 2024-10-17T15:48:32Z

src/stream/src/executor/source/source_backfill_executor.rs

@@ -206,6 +216,8 @@ impl BackfillStage {
    /// Updates backfill states and returns whether the row backfilled from external system is visible.
    fn handle_backfill_row(&mut self, split_id: &str, offset: &str) -> bool {
        let state = self.states.get_mut(split_id).unwrap();
+        state.num_consumed_rows += 1;


Now we need to +=1 since we need to count and persist separately for each split.

xxchan · 2024-10-17T15:58:33Z

src/stream/src/executor/source/source_backfill_executor.rs

-                                        total_backfilled_rows,
+                                        backfill_stage.total_backfilled_rows(),


Now we calculate per-split cnt, and sum here

BugenZhao · 2024-10-18T08:57:46Z

And we have to persist it just for SHOW JOBS.

You mean it's just for observability/debugging purposes?

xxchan · 2024-10-18T09:04:34Z

And we have to persist it just for SHOW JOBS.

You mean it's just for observability/debugging purposes?

Yes.

xxchan · 2024-10-21T01:43:03Z

Hey @BugenZhao @kwannoel , would you mind taking a look at the downstack PR #18925? 🤣

xxchan · 2024-10-21T03:39:39Z

e2e_test/source_inline/kafka/shared_source.slt.serial

+2 {"v1": 3, "v2": "c"}
+3 {"v1": 4, "v2": "d"}
+3 {"v1": 4, "v2": "d"}


produce more data to make partitions look different

xxchan · 2024-10-21T03:44:12Z

Inspired by #18925 (comment), I also persist target_offset in state table to make selecting from state table more informative. (SHOW JOBS is not affected)

👍 if it looks good @BugenZhao @kwannoel 👀

xxchan · 2024-10-21T03:45:43Z

BTW, I just realized that we can merge this PR first, not necessarily wait for #18925

kwannoel · 2024-10-21T03:51:05Z

e2e_test/source_inline/kafka/shared_source.slt.serial

+0,"{""num_consumed_rows"": 0, ""state"": ""Finished"", ""target_offset"": null}"
+1,"{""num_consumed_rows"": 0, ""state"": ""Finished"", ""target_offset"": null}"
+2,"{""num_consumed_rows"": 0, ""state"": ""Finished"", ""target_offset"": null}"
+3,"{""num_consumed_rows"": 0, ""state"": ""Finished"", ""target_offset"": null}"


Seems like we don't have any tests to check the output of show jobs?

The DDL finishes immediately. It seems not easy to test show jobs (and it has non deterministic results).

kwannoel · 2024-10-21T04:07:56Z

Actually do we still need to persist backfilled rows? Since we can just use offset to show progress.

xxchan · 2024-10-21T04:31:42Z

Actually do we still need to persist backfilled rows? Since we can just use offset to show progress.

I'm thinking num_backfilled_rows is still valuable now, which provides a different dimension of information as offsets.
See #18925 (comment)

How do you think?

xxchan · 2024-10-21T04:40:49Z

We may also deprecate some fields in the future if they are not useful.

Signed-off-by: xxchan <[email protected]>

#19039) Signed-off-by: xxchan <[email protected]> Co-authored-by: xxchan <[email protected]>

github-actions bot added the type/feature label Oct 17, 2024

xxchan mentioned this pull request Oct 17, 2024

feat: improve progress msg in SHOW JOBS for source backfill #18925

Merged

9 tasks

xxchan marked this pull request as ready for review October 17, 2024 15:44

xxchan force-pushed the 10-17-feat_persist_processed_rows_for_sourcebackfill_for_show_jobs branch from 6436f64 to 1fa501c Compare October 17, 2024 15:45

xxchan commented Oct 17, 2024

View reviewed changes

xxchan force-pushed the 09-06-feat_improve_progress_msg_for_source_backfill branch from c9d083b to 16ae682 Compare October 17, 2024 15:55

xxchan force-pushed the 10-17-feat_persist_processed_rows_for_sourcebackfill_for_show_jobs branch 2 times, most recently from 7c2e849 to e6086ef Compare October 17, 2024 15:57

xxchan commented Oct 17, 2024

View reviewed changes

xxchan force-pushed the 09-06-feat_improve_progress_msg_for_source_backfill branch from 16ae682 to 3498778 Compare October 18, 2024 00:16

xxchan force-pushed the 10-17-feat_persist_processed_rows_for_sourcebackfill_for_show_jobs branch from e6086ef to 09a9737 Compare October 18, 2024 00:16

xxchan force-pushed the 09-06-feat_improve_progress_msg_for_source_backfill branch from 3498778 to 4a18292 Compare October 18, 2024 00:40

xxchan force-pushed the 10-17-feat_persist_processed_rows_for_sourcebackfill_for_show_jobs branch from 09a9737 to 7fa4f3f Compare October 18, 2024 00:40

xxchan force-pushed the 09-06-feat_improve_progress_msg_for_source_backfill branch from 4a18292 to 1c50a1b Compare October 18, 2024 03:15

xxchan force-pushed the 10-17-feat_persist_processed_rows_for_sourcebackfill_for_show_jobs branch from 7fa4f3f to b08b8af Compare October 18, 2024 03:16

xxchan requested review from chenzl25, fuyufjh and BugenZhao October 18, 2024 03:17

kwannoel approved these changes Oct 18, 2024

View reviewed changes

xxchan force-pushed the 09-06-feat_improve_progress_msg_for_source_backfill branch from 1c50a1b to eaa34e4 Compare October 18, 2024 07:04

xxchan force-pushed the 10-17-feat_persist_processed_rows_for_sourcebackfill_for_show_jobs branch from b08b8af to ef793bd Compare October 18, 2024 07:04

xxchan force-pushed the 09-06-feat_improve_progress_msg_for_source_backfill branch from eaa34e4 to 0f6248b Compare October 18, 2024 07:27

xxchan force-pushed the 10-17-feat_persist_processed_rows_for_sourcebackfill_for_show_jobs branch 2 times, most recently from 45f6490 to 874ca44 Compare October 18, 2024 07:33

BugenZhao approved these changes Oct 18, 2024

View reviewed changes

xxchan added the need-cherry-pick-release-2.1 label Oct 21, 2024

xxchan force-pushed the 09-06-feat_improve_progress_msg_for_source_backfill branch from 7d123f2 to d3ef9bb Compare October 21, 2024 01:47

xxchan force-pushed the 10-17-feat_persist_processed_rows_for_sourcebackfill_for_show_jobs branch from 874ca44 to f0130cd Compare October 21, 2024 01:47

xxchan commented Oct 21, 2024

View reviewed changes

kwannoel reviewed Oct 21, 2024

View reviewed changes

kwannoel approved these changes Oct 21, 2024

View reviewed changes

xxchan added 2 commits October 21, 2024 16:23

feat: persist processed rows for SourceBackfill for SHOW JOBS

85aa5d3

also persist target_offset in state table

4d00aac

xxchan force-pushed the 10-17-feat_persist_processed_rows_for_sourcebackfill_for_show_jobs branch from f7949f1 to 4d00aac Compare October 21, 2024 08:48

xxchan changed the base branch from 09-06-feat_improve_progress_msg_for_source_backfill to main October 21, 2024 08:48

remove update

b7f1b41

Signed-off-by: xxchan <[email protected]>

xxchan enabled auto-merge October 21, 2024 08:57

xxchan added this pull request to the merge queue Oct 21, 2024

graphite-app bot requested a review from a team October 21, 2024 09:36

Merged via the queue into main with commit 3e7fffd Oct 21, 2024
30 of 31 checks passed

xxchan deleted the 10-17-feat_persist_processed_rows_for_sourcebackfill_for_show_jobs branch October 21, 2024 10:02

github-actions bot pushed a commit that referenced this pull request Oct 21, 2024

feat: persist processed rows for SourceBackfill for SHOW JOBS (#18983)

8d4b081

Signed-off-by: xxchan <[email protected]>

github-actions bot mentioned this pull request Oct 21, 2024

feat: persist processed rows for SourceBackfill for SHOW JOBS (#18983) #19039

Merged

github-merge-queue bot pushed a commit that referenced this pull request Oct 21, 2024

feat: persist processed rows for SourceBackfill for SHOW JOBS (#18983) (

4fdb5ce

#19039) Signed-off-by: xxchan <[email protected]> Co-authored-by: xxchan <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: persist processed rows for SourceBackfill for SHOW JOBS #18983

feat: persist processed rows for SourceBackfill for SHOW JOBS #18983

xxchan commented Oct 17, 2024 •

edited

Loading

xxchan commented Oct 17, 2024 •

edited

Loading

xxchan Oct 17, 2024 •

edited

Loading

xxchan Oct 17, 2024 •

edited

Loading

xxchan Oct 17, 2024

xxchan Oct 17, 2024

BugenZhao commented Oct 18, 2024

xxchan commented Oct 18, 2024

xxchan commented Oct 21, 2024

xxchan Oct 21, 2024

xxchan commented Oct 21, 2024 •

edited

Loading

xxchan commented Oct 21, 2024

kwannoel Oct 21, 2024

xxchan Oct 21, 2024 •

edited

Loading

kwannoel commented Oct 21, 2024

xxchan commented Oct 21, 2024

xxchan commented Oct 21, 2024

		total_backfilled_rows,
		backfill_stage.total_backfilled_rows(),

feat: persist processed rows for SourceBackfill for SHOW JOBS #18983

feat: persist processed rows for SourceBackfill for SHOW JOBS #18983

Conversation

xxchan commented Oct 17, 2024 • edited Loading

What's changed and what's your intention?

Checklist

Documentation

Release note

xxchan commented Oct 17, 2024 • edited Loading

xxchan Oct 17, 2024 • edited Loading

Choose a reason for hiding this comment

xxchan Oct 17, 2024 • edited Loading

Choose a reason for hiding this comment

xxchan Oct 17, 2024

Choose a reason for hiding this comment

xxchan Oct 17, 2024

Choose a reason for hiding this comment

BugenZhao commented Oct 18, 2024

xxchan commented Oct 18, 2024

xxchan commented Oct 21, 2024

xxchan Oct 21, 2024

Choose a reason for hiding this comment

xxchan commented Oct 21, 2024 • edited Loading

xxchan commented Oct 21, 2024

kwannoel Oct 21, 2024

Choose a reason for hiding this comment

xxchan Oct 21, 2024 • edited Loading

Choose a reason for hiding this comment

kwannoel commented Oct 21, 2024

xxchan commented Oct 21, 2024

xxchan commented Oct 21, 2024

xxchan commented Oct 17, 2024 •

edited

Loading

xxchan commented Oct 17, 2024 •

edited

Loading

xxchan Oct 17, 2024 •

edited

Loading

xxchan Oct 17, 2024 •

edited

Loading

xxchan commented Oct 21, 2024 •

edited

Loading

xxchan Oct 21, 2024 •

edited

Loading