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(frontend): support alter rate limit in mv, source, table with source #16399

Merged
merged 26 commits into from
Apr 29, 2024

Conversation

kwannoel
Copy link
Contributor

@kwannoel kwannoel commented Apr 19, 2024

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

What's changed and what's your intention?

Support frontend parts for altering rate limit with source, source with table, mv.
Only source, source with table now fully support for ALTER SET STREAMING_RATE_LIMIT TO N. Look at the slt test for more information.

Changes for MV (will be done in a future PR)

For MV, this is not fully supported yet. The reason is that we need specific information to identify an MV besides its name (which alter mv provides). We need the schema, and database as well.

These parameters can be provided from the catalog in meta. Currently, when the MV is in Creating state, it is persisted on the meta side. However, we currently don't propagate it back to frontend, to make it "invisible" until backfill completes.
We can propagate it back to frontend, and update catalog accessors to mask Creating catalogs.

Then we can then access these Creating catalogs in frontend to get the necessary info to alter rate limit.

In this PR, the behaviour of MV being backfilled is to reject the alter rate limit command, and inform the user that its catalog is not found.

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

Supports ALTER STREAMING_RATE_LIMIT SET = default | N for source, table with source.

Example:

create table kafka_source (v1 int) with (
  connector = 'kafka',
  topic = 'kafka_source',
  properties.bootstrap.server = 'localhost:29092',
  scan.startup.mode = 'earliest',
  streaming_rate_limit = 0
) FORMAT PLAIN ENCODE JSON

alter table kafka_source set streaming_rate_limit to default;

alter table kafka_source set streaming_rate_limit to 1000;

@kwannoel kwannoel marked this pull request as draft April 19, 2024 06:59
@kwannoel kwannoel changed the title feat(frontend, meta): support alter mv feat(frontend, meta): support alter mv, source, table with source Apr 22, 2024
@kwannoel kwannoel changed the title feat(frontend, meta): support alter mv, source, table with source feat(frontend): support alter rate limit in mv, source, table with source Apr 22, 2024
@kwannoel kwannoel marked this pull request as ready for review April 22, 2024 06:33
@kwannoel kwannoel added the user-facing-changes Contains changes that are visible to users label Apr 22, 2024
@kwannoel
Copy link
Contributor Author

Bump, please leave a review.

@kwannoel
Copy link
Contributor Author

kwannoel commented Apr 24, 2024

Just found 2 bugs.
1. Table rate limit does not get altered.
2. Source rate limit is not applied initially.

Investigating.

  • Edit: This is likely because rate_limit is stored in StreamSource, and not persisted in SourceCatalog, so it does not apply it subsequently on MVs. That's a separate issue. For now I workaround this behaviour by using session variable to set it instead.

  • Also seems like these tests are not running need to patch e2e-source-tests.sh to run these tests.

@kwannoel

This comment was marked as outdated.

@kwannoel
Copy link
Contributor Author

Ready for review again.

@kwannoel kwannoel added this pull request to the merge queue Apr 29, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 29, 2024
@kwannoel kwannoel enabled auto-merge April 29, 2024 03:18
@kwannoel kwannoel added this pull request to the merge queue Apr 29, 2024
Merged via the queue into main with commit 03e6283 Apr 29, 2024
30 of 31 checks passed
@kwannoel kwannoel deleted the kwannoel/support-alter-mv branch April 29, 2024 06:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants