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

feat(meta): make blocking sink default + support background sink #16249

Merged
merged 9 commits into from
Apr 17, 2024

Conversation

kwannoel
Copy link
Contributor

@kwannoel kwannoel commented Apr 11, 2024

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

What's changed and what's your intention?

Part of #15587

  1. Change sink to block by default again, until backfill completes.
  2. If background ddl set, then we let create sink immediately succeed.

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

This PR contains user-facing-changes to the behaviour of sink creation.
After this PR, we will block until backfill completes.
Before this PR, we will immediately let sink finish creating, once first barrier completes. After this PR, if this is the desired behaviour, set BACKGROUND_DDL=true.

@kwannoel kwannoel changed the title feat(meta): block sink if it is foreground stream job feat(meta): make blocking sink default + support background sink Apr 11, 2024
@kwannoel kwannoel marked this pull request as ready for review April 11, 2024 07:55
@kwannoel kwannoel added the user-facing-changes Contains changes that are visible to users label Apr 15, 2024
@kwannoel
Copy link
Contributor Author

Bump, seems like this PR was forgotten

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.

Rest LGTM

Comment on lines +495 to 498
if *ddl_type == DdlType::Sink && *create_type == CreateType::Background {
// We return the original tracking job immediately.
// This is because sink can be decoupled with backfill progress.
// We don't need to wait for sink to finish backfill.
Copy link
Member

Choose a reason for hiding this comment

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

Is this path still necessary? Can we now simply treat sinks like materialized views?

Copy link
Contributor Author

@kwannoel kwannoel Apr 16, 2024

Choose a reason for hiding this comment

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

I suppose so. But it will require more refactoring of code.

Sink is unique from materialized view in that the output will immediately be visible, since it's external to the stream job.
For materialized views, we have to do something extra, which is to hide the output until backfill is complete.

As such we persist materialized views twice, when they undergo background ddl:

  1. On initial creation. (mark as Creating, i.e. invisible, but recoverable).
  2. After creation succeeds (mark as Finished, i.e. visible).

For sink we can immediately mark it as Finished (i.e. just step 2), we can't do that for MVs.

For Index, we have to also go through the same flow as materialized view. When I support Index I will also review to see if Sink can be supported likewise and unify more branches of code.

For now I think this is acceptable approach which requires least changes, and with simple concept.

Copy link
Member

Choose a reason for hiding this comment

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

For sink we can immediately mark it as Finished (i.e. just step 2), we can't do that for MVs.

This is interesting to realize. 😄

e2e_test/backfill/sink/create_sink.slt Show resolved Hide resolved
@kwannoel kwannoel enabled auto-merge April 16, 2024 10:05
@kwannoel kwannoel added this pull request to the merge queue Apr 17, 2024
Merged via the queue into main with commit ffd4194 Apr 17, 2024
29 of 30 checks passed
@kwannoel kwannoel deleted the kwannoel/block-sink branch April 17, 2024 03:19
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.

None yet

2 participants