feat(source): create source stream with retry during recovery #19805

StrikeW · 2024-12-16T03:02:17Z

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

What's changed and what's your intention?

Previously we reverted the retry for source executor #19754. This PR we re-introduce the retry mechanism but only during recovery process. Because when source job create in the first time, it should fail if cannot reach the upstream system.

And we merge the patch #19714 into this PR

passed main-cron https://buildkite.com/risingwavelabs/main-cron/builds/4092
close #19681

Checklist

I have written necessary rustdoc comments.
I have added necessary unit tests and integration tests.
I have added test labels as necessary.
I have added fuzzing tests or opened an issue to track them.
My PR contains breaking changes.
My PR changes performance-critical code, so I will run (micro) benchmarks and present the results.
My PR contains critical fixes that are necessary to be merged into the latest release.

Documentation

My PR needs documentation updates.

Release note

xxchan

We may remove the workaround here to get it tested.

risingwave/e2e_test/source_inline/kafka/issue_19563.slt.serial

Lines 49 to 51 in 1e24ca4

    
           # XXX: wait until source reader is ready. then produce data. 
        
           # This is a temporary workaround for a data loss bug https://github.com/risingwavelabs/risingwave/issues/19681#issuecomment-2532183002 
        
           sleep 2s

BTW, is there any test case covered the CDC sync issue #19681?

xxchan

Code LGTM

StrikeW · 2024-12-16T06:30:27Z

We may remove the workaround here to get it tested.

risingwave/e2e_test/source_inline/kafka/issue_19563.slt.serial

Lines 49 to 51 in 1e24ca4

# XXX: wait until source reader is ready. then produce data.

# This is a temporary workaround for a data loss bug https://github.com/risingwavelabs/risingwave/issues/19681#issuecomment-2532183002

sleep 2s

BTW, is there any test case covered the CDC sync issue #19681?

I am testing #19681 with the test pipeline https://buildkite.com/risingwave-test/ch-benchmark-pg-cdc-shared-source

src/stream/src/executor/source/source_executor.rs

StrikeW · 2024-12-17T02:29:20Z

We may remove the workaround here to get it tested.

risingwave/e2e_test/source_inline/kafka/issue_19563.slt.serial

Lines 49 to 51 in 1e24ca4

# XXX: wait until source reader is ready. then produce data.

# This is a temporary workaround for a data loss bug https://github.com/risingwavelabs/risingwave/issues/19681#issuecomment-2532183002

sleep 2s

BTW, is there any test case covered the CDC sync issue #19681?

I am testing #19681 with the test pipeline https://buildkite.com/risingwave-test/ch-benchmark-pg-cdc-shared-source

Test passed https://buildkite.com/risingwave-test/ch-benchmark-pg-cdc-shared-source/builds/133

hzxa21 · 2024-12-17T04:55:20Z

...onnector-service/src/main/java/com/risingwave/connector/source/common/PostgresValidator.java

+                LOG.info(
+                        "creating cdc source job: publication '{}' doesn't exist, creating...",
+                        pubName);
+                DbzSourceUtils.createPostgresPublicationInValidate(userProps);


Why do we need to create the publication in meta instead of doing it in the source reader? I thought the CREATE SOURCE logic is unchanged (block the 1st barrier until source reader creation succeeds) before and after this PR.

You can see in the source_executor.rs, we forward the initial barrier first before we create the source reader. IIRC, after Meta collected the initial barrier the CREATE SOURCE will return success, and then following CREATE TABLE command can be issued to Meta. So during validation of CREATE TABLE, the pg publication may not have been created.

I think this race exist for a long time ago, not the new problem introduced by this PR.

hzxa21 · 2024-12-17T04:59:36Z

We may remove the workaround here to get it tested.

risingwave/e2e_test/source_inline/kafka/issue_19563.slt.serial

Lines 49 to 51 in 1e24ca4

# XXX: wait until source reader is ready. then produce data.

# This is a temporary workaround for a data loss bug https://github.com/risingwavelabs/risingwave/issues/19681#issuecomment-2532183002

sleep 2s

BTW, is there any test case covered the CDC sync issue #19681?

I am testing #19681 with the test pipeline https://buildkite.com/risingwave-test/ch-benchmark-pg-cdc-shared-source

Test passed https://buildkite.com/risingwave-test/ch-benchmark-pg-cdc-shared-source/builds/133

Will the ch-benchmark test the case when upstream is unavailable? If not, I think we still in lack of a test case for that.

hzxa21 · 2024-12-17T14:11:50Z

Link to the context: #19681 (comment)

hzxa21

LGTM

#19856) Co-authored-by: StrikeW <[email protected]>

source with retry in recovery

efec2f0

github-actions bot added the type/feature label Dec 16, 2024

StrikeW added ci/run-build ci/run-build-other ci/run-e2e-source-tests ci/run-check ci/main-cron/run-selected ci/pr/run-selected ci/main-cron/run-e2e-source-tests ci/main-cron/skip-ci and removed type/feature labels Dec 16, 2024

minor

381a985

github-actions bot added the type/feature label Dec 16, 2024

minor

84822f4

StrikeW marked this pull request as ready for review December 16, 2024 03:48

StrikeW removed ci/run-build ci/run-build-other ci/run-e2e-source-tests ci/run-check ci/main-cron/run-selected ci/pr/run-selected ci/main-cron/run-e2e-source-tests ci/main-cron/skip-ci labels Dec 16, 2024

Merge remote-tracking branch 'origin/main' into siyuan/source-retry

7a8cd83

StrikeW requested review from hzxa21 and xxchan December 16, 2024 04:08

xxchan requested a review from BugenZhao December 16, 2024 04:46

xxchan reviewed Dec 16, 2024

View reviewed changes

StrikeW added 2 commits December 16, 2024 15:03

add log and assert

a8ef7da

minor

ccec558

hzxa21 reviewed Dec 16, 2024

View reviewed changes

src/stream/src/executor/source/source_executor.rs Outdated Show resolved Hide resolved

src/stream/src/executor/source/source_executor.rs Show resolved Hide resolved

src/stream/src/executor/source/source_executor.rs Show resolved Hide resolved

BugenZhao reviewed Dec 16, 2024

View reviewed changes

src/stream/src/executor/source/source_executor.rs Outdated Show resolved Hide resolved

StrikeW added 5 commits December 16, 2024 17:47

minor

a32ec1d

create pg pub in meta

d23834a

fix

e19afe1

add

329bdc3

Merge remote-tracking branch 'origin/main' into siyuan/source-retry

01ac23e

StrikeW requested review from xxchan and hzxa21 December 17, 2024 02:28

StrikeW added the need-cherry-pick-release-2.2 label Dec 17, 2024

hzxa21 reviewed Dec 17, 2024

View reviewed changes

StrikeW requested a review from hzxa21 December 17, 2024 05:39

remove e2e workaround

0612741

hzxa21 approved these changes Dec 17, 2024

View reviewed changes

StrikeW added 2 commits December 18, 2024 15:58

Merge remote-tracking branch 'origin/main' into siyuan/source-retry

91a6e1c

Merge remote-tracking branch 'origin/main' into siyuan/source-retry

91bada1

StrikeW enabled auto-merge December 18, 2024 08:54

StrikeW added this pull request to the merge queue Dec 18, 2024

graphite-app bot requested a review from a team December 18, 2024 09:02

Merged via the queue into main with commit fe84f69 Dec 18, 2024
28 of 29 checks passed

StrikeW deleted the siyuan/source-retry branch December 18, 2024 09:03

github-actions bot pushed a commit that referenced this pull request Dec 18, 2024

feat(source): create source stream with retry during recovery (#19805)

9305b40

github-actions bot mentioned this pull request Dec 18, 2024

feat(source): create source stream with retry during recovery (#19805) #19856

Merged

StrikeW added a commit that referenced this pull request Dec 19, 2024

feat(source): create source stream with retry during recovery (#19805) (

6380001

#19856) Co-authored-by: StrikeW <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(source): create source stream with retry during recovery #19805

feat(source): create source stream with retry during recovery #19805

StrikeW commented Dec 16, 2024 •

edited

Loading

xxchan left a comment

xxchan left a comment

StrikeW commented Dec 16, 2024

StrikeW commented Dec 17, 2024

hzxa21 Dec 17, 2024

StrikeW Dec 17, 2024

StrikeW Dec 17, 2024

hzxa21 commented Dec 17, 2024

hzxa21 commented Dec 17, 2024

hzxa21 left a comment

	# XXX: wait until source reader is ready. then produce data.
	# This is a temporary workaround for a data loss bug https://github.com/risingwavelabs/risingwave/issues/19681#issuecomment-2532183002
	sleep 2s

feat(source): create source stream with retry during recovery #19805

feat(source): create source stream with retry during recovery #19805

Conversation

StrikeW commented Dec 16, 2024 • edited Loading

What's changed and what's your intention?

Checklist

Documentation

xxchan left a comment

Choose a reason for hiding this comment

xxchan left a comment

Choose a reason for hiding this comment

StrikeW commented Dec 16, 2024

StrikeW commented Dec 17, 2024

hzxa21 Dec 17, 2024

Choose a reason for hiding this comment

StrikeW Dec 17, 2024

Choose a reason for hiding this comment

StrikeW Dec 17, 2024

Choose a reason for hiding this comment

hzxa21 commented Dec 17, 2024

hzxa21 commented Dec 17, 2024

hzxa21 left a comment

Choose a reason for hiding this comment

StrikeW commented Dec 16, 2024 •

edited

Loading