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): add max_batch_queries_per_frontend_node config #15574

Merged
merged 3 commits into from
Mar 8, 2024

Conversation

kwannoel
Copy link
Contributor

@kwannoel kwannoel commented Mar 8, 2024

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

What's changed and what's your intention?

We currently have distributed_query_limit, but that just limits the number of queries per session.
What we want instead is to limit the number of concurrent batch queries cluster wide.

The usecase for it is when there's some batch query which triggers OOM, we want to limit the number of this batch query.

Because meta node doesn't really manage batch queries, we have to do it elsewhere. We choose to do it in frontend, because there's typically only 1 frontend node, so effectively, it's a cluster wide limit on the number of batch queries.

We simply add the permit to QueryExecution which must be dropped (deleted), when the execution ends either due to cancel, errors, or completion.

I have tested it locally in the following way:

./risedev d full-with-batch-query-limit

Execute:

CREATE TABLE t(v1 int);
INSERT INTO t select 1 from generate_series(1, 10000);

Start 2 long running queries:

./risedev psql -c 'select count(*) from t x join t y using(v1); &
./risedev psql -c 'select count(*) from t x join t y using(v1); &

Execute another query in local mode:

select 1;

Can execute without issues.

Execute a query which should be pretty fast:

select count(*) from t;

It will now take a long time.

Cancel the previous 2 queries, and we can continue running queries.

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.

@kwannoel kwannoel enabled auto-merge March 8, 2024 13:27
@kwannoel kwannoel disabled auto-merge March 8, 2024 13:27
@kwannoel
Copy link
Contributor Author

kwannoel commented Mar 8, 2024

Merge it in first, urgent for customer's usecase.

@kwannoel kwannoel enabled auto-merge March 8, 2024 13:27
@kwannoel kwannoel disabled auto-merge March 8, 2024 13:36
@kwannoel
Copy link
Contributor Author

kwannoel commented Mar 8, 2024

I will change the semantics slightly. Since these batch queries are not directly ran by a user, but rather by dashboard, I think we can just let the queries block the session, since they will not be retried automatically.

@kwannoel kwannoel added this pull request to the merge queue Mar 8, 2024
Merged via the queue into main with commit f796432 Mar 8, 2024
26 of 27 checks passed
@kwannoel kwannoel deleted the kwannoel/limit-queries branch March 8, 2024 14:52
kwannoel added a commit that referenced this pull request Mar 8, 2024
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