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: deny create MV on shared CDC source #15635

Merged
merged 5 commits into from
Mar 15, 2024

Conversation

xxchan
Copy link
Member

@xxchan xxchan commented Mar 12, 2024

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

What's changed and what's your intention?

fix #13955

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.

Copy link
Member Author

xxchan commented Mar 12, 2024

@github-actions github-actions bot added the type/fix Bug fix label Mar 12, 2024
@xxchan xxchan marked this pull request as ready for review March 12, 2024 08:08
@@ -100,6 +101,10 @@ pub trait WithPropertiesExt: Get {
connector.contains("-cdc")
}

fn is_shared_cdc_source(&self) -> bool {
self.is_cdc_connector() && CdcTableType::from_properties(self).can_backfill()
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure what's the case for non-shared CDC ('citus-cdc'). Will CREATE SOURCE for it be rejected (and when?)

Copy link
Contributor

@StrikeW StrikeW Mar 13, 2024

Choose a reason for hiding this comment

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

Actually we cannot distinguish whether it is going to create a share source from properties only. is_shared_cdc_source should not provided in the trait.

Copy link
Member Author

@xxchan xxchan Mar 13, 2024

Choose a reason for hiding this comment

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

Can you elaborate? This is just extracted from the current implementation in create_source. What it should be like?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, because we do this check in create_source, so the context is to create a Source. But if without this context, we cannot distinguish it. is_shared_cdc_source in the trait can be called in other place.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fair enough.

BTW, what are the other cases? When don't we create shared CDC source for mysql-cdc? (Do you mean CREATE TABLE directly? Any other cases?)

And the original question: Can we CREATE SOURCE for citus-cdc? If not, where is it rejected?

Copy link
Contributor

Choose a reason for hiding this comment

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

BTW, what are the other cases? When don't we create shared CDC source for mysql-cdc? (Do you mean CREATE TABLE directly? Any other cases?)

CREATE TABLE directly, no other cases.

And the original question: Can we CREATE SOURCE for citus-cdc? If not, where is it rejected?

We can't. But it seems we didn't ban it in code.

   // gated the feature with a session variable
    let create_cdc_source_job = if with_properties.is_cdc_connector() {
        CdcTableType::from_properties(&with_properties).can_backfill()
    } else {
        false
    };

Copy link
Member Author

Choose a reason for hiding this comment

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

I renamed the function to is_backfillable_cdc_connector. I think there won't be any confusion now.

For BoundSource, we can call its method is_shared_cdc_source

Copy link
Member Author

Choose a reason for hiding this comment

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

For BoundSource, we can call its method is_shared_cdc_source

Seems not true. CREATE TABLE will also have a Source node.. Let me just call it is_backfillable_cdc_connector

Base automatically changed from xxchan/fixed-xerinae to main March 12, 2024 09:12
@xxchan xxchan force-pushed the 03-12-fix_deny_create_MV_on_shared_CDC_source branch from 92e20d1 to 9c44201 Compare March 12, 2024 09:21
@xxchan xxchan mentioned this pull request Mar 12, 2024
11 tasks
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.

the impl LGTM may ask @StrikeW whether this approach meets expectation

Copy link
Contributor

@StrikeW StrikeW left a comment

Choose a reason for hiding this comment

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

LGTM!

@xxchan xxchan enabled auto-merge March 14, 2024 09:29
@xxchan xxchan added this pull request to the merge queue Mar 15, 2024
Copy link
Member Author

xxchan commented Mar 15, 2024

Merge activity

  • Mar 15, 2:55 PM GMT+8: Graphite failed to merge this pull request due to This repository has GitHub's merge queue enabled, which is currently incompatible with Graphite. Try your merge again, or report a bug if you see this consistently.

Merged via the queue into main with commit e53896a Mar 15, 2024
27 of 28 checks passed
@xxchan xxchan deleted the 03-12-fix_deny_create_MV_on_shared_CDC_source branch March 15, 2024 07:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/fix Bug fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

panic when create mv on mysql-cdc source
3 participants