Skip to content
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

fix(cdc): ensure connector is inited after the CREATE TABLE is finished #13130

Merged
merged 4 commits into from
Nov 4, 2023

Conversation

StrikeW
Copy link
Contributor

@StrikeW StrikeW commented Oct 30, 2023

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

What's changed and what's your intention?

  • Make sure connector is inited before yielding the initial barrier (injected by the CREATE TABLE). Otherwise, the connector may start to run even after a DROP TABLE has been executed and cause unexpected race condition happen on upstream DB.
  • other: migrate the -Ddebezium.embedded.shutdown.pause.before.interrupt.ms to embedded jvm

Fix the unstable citus-cdc integration test

Checklist

  • I have written necessary rustdoc comments
  • I have added necessary unit tests and integration tests
  • 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.

@github-actions github-actions bot added the type/fix Bug fix label Oct 30, 2023
@BugenZhao
Copy link
Member

Will this be somehow related to #12476?

@StrikeW
Copy link
Contributor Author

StrikeW commented Oct 30, 2023

Will this be somehow related to #12476?

Didn't look into this issue yet, will do later.

@StrikeW StrikeW changed the title fix(cdc): Make sure connector is inited before yielding the initial barrier. fix(cdc): (WIP) Make sure connector is inited before yielding the initial barrier. Oct 30, 2023
@StrikeW StrikeW marked this pull request as draft November 1, 2023 02:22
@StrikeW StrikeW changed the title fix(cdc): (WIP) Make sure connector is inited before yielding the initial barrier. fix(cdc): ensure connector is inited before yielding the initial barrier. Nov 1, 2023
@StrikeW StrikeW changed the title fix(cdc): ensure connector is inited before yielding the initial barrier. fix(cdc): ensure connector is inited after the CREATE TABLE is finished Nov 1, 2023
@StrikeW StrikeW force-pushed the siyuan/init-connector-in-source branch from 7e108da to eff2edd Compare November 2, 2023 03:47
@StrikeW StrikeW force-pushed the siyuan/init-connector-in-source branch from eff2edd to cf0b662 Compare November 2, 2023 03:54
@StrikeW StrikeW marked this pull request as ready for review November 2, 2023 03:54
@StrikeW StrikeW force-pushed the siyuan/init-connector-in-source branch from cf0b662 to b6bc630 Compare November 2, 2023 04:06
@StrikeW StrikeW force-pushed the siyuan/init-connector-in-source branch from b6bc630 to 777557d Compare November 2, 2023 04:13
@tabVersion tabVersion self-requested a review November 2, 2023 04:43
@StrikeW StrikeW requested review from hzxa21 and BugenZhao November 2, 2023 07:28
Copy link

codecov bot commented Nov 2, 2023

Codecov Report

Merging #13130 (85f8b63) into main (b7195f8) will increase coverage by 0.04%.
Report is 21 commits behind head on main.
The diff coverage is 1.42%.

@@            Coverage Diff             @@
##             main   #13130      +/-   ##
==========================================
+ Coverage   68.10%   68.14%   +0.04%     
==========================================
  Files        1507     1507              
  Lines      255634   255674      +40     
==========================================
+ Hits       174104   174237     +133     
+ Misses      81530    81437      -93     
Flag Coverage Δ
rust 68.14% <1.42%> (+0.04%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
src/source/src/connector_source.rs 66.66% <100.00%> (-1.71%) ⬇️
src/connector/src/source/cdc/split.rs 21.01% <0.00%> (+0.30%) ⬆️
src/connector/src/source/cdc/mod.rs 11.66% <0.00%> (-1.55%) ⬇️
src/jni_core/src/jvm_runtime.rs 0.00% <0.00%> (ø)
src/connector/src/source/cdc/source/reader.rs 0.00% <0.00%> (ø)

... and 21 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Member

@BugenZhao BugenZhao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM for the reader part

src/connector/src/source/cdc/source/reader.rs Show resolved Hide resolved
Copy link
Contributor

@tabVersion tabVersion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reset LGTM

)
.await
}
create_split_reader(*props, splits, parser_config, source_ctx, data_gen_columns)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no await here? I thought it is about to wait till recv handshake resp from embedded dbz

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We has await on the outer try_join_all

// In RisingWave we assume the upstream changelog may contain duplicate events and
// handle conflicts in the mview operator, thus we don't need to obey the above
// instructions. So we decrease the wait time here to reclaim jvm thread faster.
.option("-Ddebezium.embedded.shutdown.pause.before.interrupt.ms=1");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw can you share more about the param?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@StrikeW StrikeW added this pull request to the merge queue Nov 4, 2023
Merged via the queue into main with commit 68f1a67 Nov 4, 2023
12 of 13 checks passed
@StrikeW StrikeW deleted the siyuan/init-connector-in-source branch November 4, 2023 02:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants