feat(cdc): support transaction for shared cdc source #14375

StrikeW · 2024-01-05T05:27:31Z

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

What's changed and what's your intention?

close #14348

Add transaction metadata parsing logic to PlainParser for the CDC source job, which fixed to plain json format.
Add more solid tests for emitting transactional chunk
Fix transaction id parsing logic for specific database

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.

User can specify transactional = true parameter to the WITH options to preserve upstream transaction semantic when create a CDC source.

tabVersion

Add transaction metadata parsing logic to PlainParser for the CDC source job, which fixed to plain json format.

I am a little worried that we are mixing too much logic into plain parser. shall we make a new format dedicated to CDC source?

StrikeW · 2024-01-05T05:40:32Z

Add transaction metadata parsing logic to PlainParser for the CDC source job, which fixed to plain json format.

I am a little worried that we are mixing too much logic into plain parser. shall we make a new format dedicated to CDC source?

Hmm, the modification to plain parser is small, only add a new branch to parse transaction metadata if it needs to.
https://github.com/risingwavelabs/risingwave/pull/14375/files#diff-8c7e5305d8e1796a4ccc76a9522ccb881ea31abeeb6d8010427679c5d9fa801fR91-R95

tabVersion · 2024-01-05T06:35:34Z

Add transaction metadata parsing logic to PlainParser for the CDC source job, which fixed to plain json format.

I am a little worried that we are mixing too much logic into plain parser. shall we make a new format dedicated to CDC source?

Hmm, the modification to plain parser is small, only add a new branch to parse transaction metadata if it needs to.
https://github.com/risingwavelabs/risingwave/pull/14375/files#diff-8c7e5305d8e1796a4ccc76a9522ccb881ea31abeeb6d8010427679c5d9fa801fR91-R95

Yes, I am ok with the current impl, just wonder if you have such plan.

StrikeW · 2024-01-05T06:39:51Z

Add transaction metadata parsing logic to PlainParser for the CDC source job, which fixed to plain json format.

I am a little worried that we are mixing too much logic into plain parser. shall we make a new format dedicated to CDC source?

Hmm, the modification to plain parser is small, only add a new branch to parse transaction metadata if it needs to.
https://github.com/risingwavelabs/risingwave/pull/14375/files#diff-8c7e5305d8e1796a4ccc76a9522ccb881ea31abeeb6d8010427679c5d9fa801fR91-R95

Yes, I am ok with the current impl, just wonder if you have such plan.

Right now the modification is small, so I think it is ok to just extend the plain parser. If there are more features need to add to the plain parser (e.g. handle schema change events), we can define a new parser and format for it at that time.

tabVersion

LGTM

BugenZhao

May I ask what's PlainParser for? Why aren't we using JsonParser here?

BugenZhao · 2024-01-08T04:37:21Z

src/connector/src/parser/plain_parser.rs

        &'a mut self,
        key: Option<Vec<u8>>,
        payload: Option<Vec<u8>>,
        writer: SourceStreamChunkRowWriter<'a>,
-    ) -> Result<()> {
+    ) -> Result<ParseResult> {
+        tracing::info!("parse_one_with_txn");


Will this be too verbose?

My bad, will remove it in a following bugfix PR.

BugenZhao · 2024-01-08T04:42:00Z

src/connector/src/parser/unified/debezium.rs

+                ConnectorProperties::PostgresCdc(_) => {
+                    let (tx_id, _) = id.split_once(':').unwrap();
+                    return Ok(TransactionControl::Begin { id: tx_id.into() });
+                }
+                ConnectorProperties::MysqlCdc(_) => return Ok(TransactionControl::Begin { id }),


Where do we depend on the value of the ID?

Currently we have a check in https://github.com/risingwavelabs/risingwave/pull/14375/files#diff-d20a701c6d209a35e84545b11ccd13ab2f332a68aa175eb4bf73edf67184e8bcR673-R675, if transaction id mismatch, it will print a warn log.

Oh, I see. IIUC, the LSN for BEGIN and COMMIT differs from each other?

Yes, lsn in commit will greater than begin.

StrikeW · 2024-01-09T01:49:07Z

May I ask what's PlainParser for? Why aren't we using JsonParser here?

It is for parsing message for source in FORMAT PLAIN format, and actually we don't have JsonParser, json is an encoding not format in our terminology.

risingwave/src/connector/src/parser/plain_parser.rs

Lines 58 to 63 in 414c6ec

    
           let payload_builder = match props.encoding_config { 
        
               EncodingProperties::Json(_) 
        
               | EncodingProperties::Protobuf(_) 
        
               | EncodingProperties::Avro(_) 
        
               | EncodingProperties::Bytes(_) => { 
        
                   AccessBuilderImpl::new_default(props.encoding_config, EncodingType::Value).await?

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

fuyufjh · 2024-01-10T03:53:17Z

User can specify transactional = true parameter to the WITH options to preserve upstream transaction semantic when create a CDC source.

Is it possible to enable this feature by default? Will this involve additional cost?

StrikeW · 2024-01-10T04:11:29Z

User can specify transactional = true parameter to the WITH options to preserve upstream transaction semantic when create a CDC source.

Is it possible to enable this feature by default? Will this involve additional cost?

The overhead should be small. But I think we should conduct a performance test to make this decision. @lmatz Does our ch-benchmark/sysbench use BEGIN and COMMIT on upstream database to generate workload? If it is we can create a transactional source and eval its performance.

fuyufjh · 2024-01-18T03:08:06Z

User can specify transactional = true parameter to the WITH options to preserve upstream transaction semantic when create a CDC source.

Is it possible to enable this feature by default? Will this involve additional cost?

The overhead should be small. But I think we should conduct a performance test to make this decision. @lmatz Does our ch-benchmark/sysbench use BEGIN and COMMIT on upstream database to generate workload? If it is we can create a transactional source and eval its performance.

Any follow-ups? @StrikeW

StrikeW · 2024-01-18T03:19:29Z

User can specify transactional = true parameter to the WITH options to preserve upstream transaction semantic when create a CDC source.

Is it possible to enable this feature by default? Will this involve additional cost?

The overhead should be small. But I think we should conduct a performance test to make this decision. @lmatz Does our ch-benchmark/sysbench use BEGIN and COMMIT on upstream database to generate workload? If it is we can create a transactional source and eval its performance.

Any follow-ups? @StrikeW

@cyliu0 is working on the test, the ETA is next week. https://github.com/risingwavelabs/risingwave-test/issues/577

StrikeW · 2024-01-26T07:52:16Z

Update the evaluation result (nightly-20240125)

The overhead is about 5% for mysql and 4% for pg roughly and it is increased inside the debezium connector.

I think the overhead is not trivial, it is better to remain the transactional to false by default. @fuyufjh

fuyufjh · 2024-01-26T07:56:41Z

I think the overhead is not trivial, it is better to remain the transactional to false by default. @fuyufjh

Thanks for the testing work!

I prefer to enable this by default so that we can claim the CDC connector is transactional. Otherwise, telling people "Yes-but" (Yes it supports transaction but needs to be enabled manually) sounds bad to me.

StrikeW · 2024-01-26T08:17:03Z

I got it. Let's do the change in v1.7.0.

StrikeW added 6 commits January 3, 2024 17:16

WIP: eval transaction

b5aedaa

Merge remote-tracking branch 'origin/main' into siyuan/cdc-transaction

dafccf6

support transaction for shared cdc source

21a60b3

minor

aac2b39

parse transaction meta based on connector type

e0f36a3

refactor

1b0b01d

StrikeW requested review from hzxa21, fuyufjh, tabVersion and BugenZhao January 5, 2024 05:27

StrikeW added the user-facing-changes Contains changes that are visible to users label Jan 5, 2024

github-actions bot added the type/feature label Jan 5, 2024

StrikeW added need-cherry-pick-release-1.6 and removed type/feature labels Jan 5, 2024

github-actions bot added the type/feature label Jan 5, 2024

StrikeW mentioned this pull request Jan 5, 2024

Tracking: Direct CDC Source Enhancement #11079

Closed

15 tasks

tabVersion reviewed Jan 5, 2024

View reviewed changes

tabVersion approved these changes Jan 5, 2024

View reviewed changes

minor

475d870

StrikeW added this pull request to the merge queue Jan 8, 2024

Merged via the queue into main with commit 10f7b77 Jan 8, 2024
29 of 30 checks passed

StrikeW deleted the siyuan/cdc-transaction branch January 8, 2024 03:48

github-actions bot pushed a commit that referenced this pull request Jan 8, 2024

feat(cdc): support transaction for shared cdc source (#14375)

bce261b

github-actions bot mentioned this pull request Jan 8, 2024

feat(cdc): support transaction for shared cdc source (#14375) #14402

Merged

BugenZhao reviewed Jan 8, 2024

View reviewed changes

xxchan mentioned this pull request Jan 9, 2024

Compute node OOM for nexmark queries with nightly-20240108 #14424

Closed

StrikeW added a commit that referenced this pull request Jan 9, 2024

feat(cdc): support transaction for shared cdc source (#14375) (#14402)

5f7068b

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

xxchan mentioned this pull request Jan 11, 2024

risingwave 1.6.0 risingwavelabs/homebrew-risingwave#22

Closed

StrikeW mentioned this pull request Jan 26, 2024

CDC Source: change the default value of transactional attribute to true #14818

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(cdc): support transaction for shared cdc source #14375

feat(cdc): support transaction for shared cdc source #14375

StrikeW commented Jan 5, 2024 •

edited

Loading

tabVersion left a comment

StrikeW commented Jan 5, 2024

tabVersion commented Jan 5, 2024

StrikeW commented Jan 5, 2024

tabVersion left a comment

BugenZhao left a comment •

edited

Loading

BugenZhao Jan 8, 2024

StrikeW Jan 8, 2024

BugenZhao Jan 8, 2024

StrikeW Jan 8, 2024

BugenZhao Jan 8, 2024

StrikeW Jan 8, 2024

StrikeW commented Jan 9, 2024

fuyufjh commented Jan 10, 2024

StrikeW commented Jan 10, 2024 •

edited

Loading

fuyufjh commented Jan 18, 2024

StrikeW commented Jan 18, 2024

StrikeW commented Jan 26, 2024 •

edited

Loading

fuyufjh commented Jan 26, 2024 •

edited

Loading

StrikeW commented Jan 26, 2024

feat(cdc): support transaction for shared cdc source #14375

feat(cdc): support transaction for shared cdc source #14375

Conversation

StrikeW commented Jan 5, 2024 • edited Loading

What's changed and what's your intention?

Checklist

Documentation

Release note

tabVersion left a comment

Choose a reason for hiding this comment

StrikeW commented Jan 5, 2024

tabVersion commented Jan 5, 2024

StrikeW commented Jan 5, 2024

tabVersion left a comment

Choose a reason for hiding this comment

BugenZhao left a comment • edited Loading

Choose a reason for hiding this comment

BugenZhao Jan 8, 2024

Choose a reason for hiding this comment

StrikeW Jan 8, 2024

Choose a reason for hiding this comment

BugenZhao Jan 8, 2024

Choose a reason for hiding this comment

StrikeW Jan 8, 2024

Choose a reason for hiding this comment

BugenZhao Jan 8, 2024

Choose a reason for hiding this comment

StrikeW Jan 8, 2024

Choose a reason for hiding this comment

StrikeW commented Jan 9, 2024

fuyufjh commented Jan 10, 2024

StrikeW commented Jan 10, 2024 • edited Loading

fuyufjh commented Jan 18, 2024

StrikeW commented Jan 18, 2024

StrikeW commented Jan 26, 2024 • edited Loading

fuyufjh commented Jan 26, 2024 • edited Loading

StrikeW commented Jan 26, 2024

StrikeW commented Jan 5, 2024 •

edited

Loading

BugenZhao left a comment •

edited

Loading

StrikeW commented Jan 10, 2024 •

edited

Loading

StrikeW commented Jan 26, 2024 •

edited

Loading

fuyufjh commented Jan 26, 2024 •

edited

Loading