Skip to content

feat(query): support cluster level concurrent limit #17778

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

Merged
merged 8 commits into from
Apr 15, 2025

Conversation

zhang2014
Copy link
Member

@zhang2014 zhang2014 commented Apr 15, 2025

I hereby agree to the terms of the CLA available at: https://docs.databend.com/dev/policies/cla/

Summary

feat(query): support cluster level concurrent limit

Tests

  • Unit Test
  • Logic Test
  • Benchmark Test
  • No Test - Explain why

Type of change

  • Bug Fix (non-breaking change which fixes an issue)
  • New Feature (non-breaking change which adds functionality)
  • Breaking Change (fix or feature that could cause existing functionality not to work as expected)
  • Documentation Update
  • Refactoring
  • Performance Improvement
  • Other (please describe):

This change is Reviewable

@zhang2014 zhang2014 requested a review from drmingdrmer April 15, 2025 03:31
@github-actions github-actions bot added the pr-feature this PR introduces a new feature to the codebase label Apr 15, 2025
Copy link

what-the-diff bot commented Apr 15, 2025

PR Summary

  • New dependency addition: The team has included 'databend-common-meta-semaphore' to our existing dependencies as specified in Cargo.toml file. This addition could mean that we're looking to use features offered by this dependency for improving our project.

  • Function update in Queries Queue Manager: The QueriesQueueManager::init function in global_services.rs file has been updated to handle config parameters asynchronously. This change allows us to improve the overall performance and efficiency by handling configuration parameters in a non-blocking manner.

  • Refactoring the Queue Manager structure: In the queue_mgr.rs file, the 'QueueManager' structure has been refactored. These changes comprise removing the semaphore field while adding permits and meta_store fields, making our Queue Manager more versatile and efficient.

  • Changes in AcquireQueueGuard and AcquireQueueFuture: For these two elements, we've replaced OwnedSemaphorePermit with a new Permit type. This adaptation should enhance our control over these components of our project.

  • Introduction of lock key generation: A novel get_lock_key method has been implemented in the QueryEntry structure, which allows generating a lock key based on the cluster's local information, enhancing data security and integrity in our project's operation.

Copy link
Member

@drmingdrmer drmingdrmer left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 4 files at r1, 8 of 8 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @zhang2014)


src/query/service/src/sessions/queue_mgr.rs line 238 at r2 (raw file):

unsafe impl Send for AcquireQueueGuard {}
unsafe impl Sync for AcquireQueueGuard {}

It's not safe to impl Sync for it. It may lead to a panic.

@zhang2014
Copy link
Member Author

It's not safe to impl Sync for it. It may lead to a panic.

The HTTP handler needs it. Though I'm not sure why. But it's safe here.

@zhang2014
Copy link
Member Author

wait #17783

@zhang2014 zhang2014 merged commit 3dc7840 into databendlabs:main Apr 15, 2025
76 of 77 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-feature this PR introduces a new feature to the codebase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants