[Coordinator throttling] Scheduling Policies for Admission Control based on worker load by prashantgolash · Pull Request #25689 · prestodb/presto

prashantgolash · 2025-08-05T06:42:12Z

Admission control scheduling policy

Logic
Gather worker overload data from the added end point in PR - https://github.com/prestodb/presto/pull/25687
Based on configured policies (cnt of overloaded workers or pct of overloaded workers) and cluster overload, queue the queries

Background
RFC PR: https://github.com/prestodb/rfcs/pull/42

Metrics on queuing due to this feature:
Added following metrics

ClusterOverloadDuration
ClusterOverloadCount

Feature flag:
Right now feature is disabled. We can use coordinator configs to enable / add thresholds

Summary by Sourcery

Add cluster overload-based admission control scheduling policy and supporting infrastructure to throttle queries based on worker load.

New Features:

Introduce ClusterResourceChecker for periodic cluster overload detection and query throttling based on worker load metrics
Implement CpuMemoryOverloadPolicy with count- and percentage-based overload thresholds and expose overload detection count and duration metrics
Add AdmissionControlBypassConfig to allow certain queries (e.g., DDL) to bypass admission control

Enhancements:

Refactor DiscoveryNodeManager and SPI to use RemoteNodeStats for asynchronous fetching of node load metrics
Integrate cluster overload checks into InternalResourceGroup scheduling logic and update eligibility propagation on overload state changes
Introduce ClusterOverloadPolicyModule and DI bindings for policy selection and checker

Tests:

Add unit tests for ClusterResourceChecker, CpuMemoryOverloadPolicy, and AdmissionControlBypassConfig
Update existing resource group and node manager tests to support clustering resource checker mocks

Chores:

Add new configuration properties for cluster-overload throttling and internal-communication stats polling intervals

facebook-github-bot · 2025-08-05T06:42:20Z

This pull request was exported from Phabricator. Differential Revision: D79470181

…sed on worker load (prestodb#25689) Summary: Admission control scheduling policy **Logic** Gather worker overload data from the added end point in D76357677 Based on configured policies (cnt of overloaded workers or pct of overloaded workers) and cluster overload, queue the queries **Background** Design doc and rational for the change - https://docs.google.com/document/d/16pEkXPzsP09ZpZ8RxqJ-n-c5kx3TFel0b8Ubx7l6v-I/edit?tab=t.0#heading=h.bdc9ugryon9z Also some follow up questions on the design review / future directions after this change - https://docs.google.com/document/d/16pEkXPzsP09ZpZ8RxqJ-n-c5kx3TFel0b8Ubx7l6v-I/edit?tab=t.0#heading=h.afichcgpu3fe **ODS Metrics on queuing due to this feature:** Right now Queued Queries ODS could be correlated with worker overload to see if this feature is getting activated. Also we log the warn logs when cluster is overloaded. I will add specific metrics for this feature before rollout as well to make debugging easy. **Feature flag:** Right now feature is disabled. We can use coordinator configs to enable / add thresholds Differential Revision: D79470181

facebook-github-bot · 2025-08-05T06:44:14Z

This pull request was exported from Phabricator. Differential Revision: D79470181

…sed on worker load (prestodb#25689) Summary: Admission control scheduling policy **Logic** Gather worker overload data from the added end point in D76357677 Based on configured policies (cnt of overloaded workers or pct of overloaded workers) and cluster overload, queue the queries **Background** Design doc and rational for the change - https://docs.google.com/document/d/16pEkXPzsP09ZpZ8RxqJ-n-c5kx3TFel0b8Ubx7l6v-I/edit?tab=t.0#heading=h.bdc9ugryon9z Also some follow up questions on the design review / future directions after this change - https://docs.google.com/document/d/16pEkXPzsP09ZpZ8RxqJ-n-c5kx3TFel0b8Ubx7l6v-I/edit?tab=t.0#heading=h.afichcgpu3fe **ODS Metrics on queuing due to this feature:** Right now Queued Queries ODS could be correlated with worker overload to see if this feature is getting activated. Also we log the warn logs when cluster is overloaded. I will add specific metrics for this feature before rollout as well to make debugging easy. **Feature flag:** Right now feature is disabled. We can use coordinator configs to enable / add thresholds Differential Revision: D79470181

facebook-github-bot · 2025-08-05T06:45:56Z

This pull request was exported from Phabricator. Differential Revision: D79470181

…sed on worker load (prestodb#25689) Summary: Admission control scheduling policy **Logic** Gather worker overload data from the added end point in D76357677 Based on configured policies (cnt of overloaded workers or pct of overloaded workers) and cluster overload, queue the queries **Background** Design doc and rational for the change - https://docs.google.com/document/d/16pEkXPzsP09ZpZ8RxqJ-n-c5kx3TFel0b8Ubx7l6v-I/edit?tab=t.0#heading=h.bdc9ugryon9z Also some follow up questions on the design review / future directions after this change - https://docs.google.com/document/d/16pEkXPzsP09ZpZ8RxqJ-n-c5kx3TFel0b8Ubx7l6v-I/edit?tab=t.0#heading=h.afichcgpu3fe **ODS Metrics on queuing due to this feature:** Right now Queued Queries ODS could be correlated with worker overload to see if this feature is getting activated. Also we log the warn logs when cluster is overloaded. I will add specific metrics for this feature before rollout as well to make debugging easy. **Feature flag:** Right now feature is disabled. We can use coordinator configs to enable / add thresholds Differential Revision: D79470181

facebook-github-bot · 2025-08-05T07:06:40Z

This pull request was exported from Phabricator. Differential Revision: D79470181

facebook-github-bot · 2025-08-05T15:21:31Z

This pull request was exported from Phabricator. Differential Revision: D79470181

…sed on worker load (prestodb#25689) Summary: Pull Request resolved: prestodb#25689 Admission control scheduling policy **Logic** Gather worker overload data from the added end point in D76357677 Based on configured policies (cnt of overloaded workers or pct of overloaded workers) and cluster overload, queue the queries **Background** Design doc and rational for the change - https://docs.google.com/document/d/16pEkXPzsP09ZpZ8RxqJ-n-c5kx3TFel0b8Ubx7l6v-I/edit?tab=t.0#heading=h.bdc9ugryon9z Also some follow up questions on the design review / future directions after this change - https://docs.google.com/document/d/16pEkXPzsP09ZpZ8RxqJ-n-c5kx3TFel0b8Ubx7l6v-I/edit?tab=t.0#heading=h.afichcgpu3fe **ODS Metrics on queuing due to this feature:** Right now Queued Queries ODS could be correlated with worker overload to see if this feature is getting activated. Also we log the warn logs when cluster is overloaded. I will add specific metrics for this feature before rollout as well to make debugging easy. **Feature flag:** Right now feature is disabled. We can use coordinator configs to enable / add thresholds Differential Revision: D79470181

linux-foundation-easycla · 2025-08-05T15:22:07Z

The committers listed above are authorized under a signed CLA.

✅ login: prashantgolash / name: PRASHANT GOLASH (95f36ae, 9bc96e3)

tdcmeehan · 2025-08-05T17:40:18Z

Can you share these documents as an RFC in our repo? https://github.com/prestodb/rfcs

facebook-github-bot · 2025-08-05T19:36:59Z

This pull request was exported from Phabricator. Differential Revision: D79470181

…sed on worker load (prestodb#25689) Summary: Pull Request resolved: prestodb#25689 Admission control scheduling policy **Logic** Gather worker overload data from the added end point in D76357677 Based on configured policies (cnt of overloaded workers or pct of overloaded workers) and cluster overload, queue the queries **Background** Design doc and rational for the change - https://docs.google.com/document/d/16pEkXPzsP09ZpZ8RxqJ-n-c5kx3TFel0b8Ubx7l6v-I/edit?tab=t.0#heading=h.bdc9ugryon9z Also some follow up questions on the design review / future directions after this change - https://docs.google.com/document/d/16pEkXPzsP09ZpZ8RxqJ-n-c5kx3TFel0b8Ubx7l6v-I/edit?tab=t.0#heading=h.afichcgpu3fe **ODS Metrics on queuing due to this feature:** Right now Queued Queries ODS could be correlated with worker overload to see if this feature is getting activated. Also we log the warn logs when cluster is overloaded. I will add specific metrics for this feature before rollout as well to make debugging easy. **Feature flag:** Right now feature is disabled. We can use coordinator configs to enable / add thresholds Differential Revision: D79470181

…sed on worker load (prestodb#25689) Summary: Admission control scheduling policy **Logic** Gather worker overload data from the added end point in D76357677 Based on configured policies (cnt of overloaded workers or pct of overloaded workers) and cluster overload, queue the queries **Background** Design doc and rational for the change - https://docs.google.com/document/d/16pEkXPzsP09ZpZ8RxqJ-n-c5kx3TFel0b8Ubx7l6v-I/edit?tab=t.0#heading=h.bdc9ugryon9z Also some follow up questions on the design review / future directions after this change - https://docs.google.com/document/d/16pEkXPzsP09ZpZ8RxqJ-n-c5kx3TFel0b8Ubx7l6v-I/edit?tab=t.0#heading=h.afichcgpu3fe **ODS Metrics on queuing due to this feature:** Right now Queued Queries ODS could be correlated with worker overload to see if this feature is getting activated. Also we log the warn logs when cluster is overloaded. I will add specific metrics for this feature before rollout as well to make debugging easy. **Feature flag:** Right now feature is disabled. We can use coordinator configs to enable / add thresholds Differential Revision: D79470181

facebook-github-bot · 2025-08-06T00:02:23Z

This pull request was exported from Phabricator. Differential Revision: D79470181

…sed on worker load (prestodb#25689) Summary: Pull Request resolved: prestodb#25689 Admission control scheduling policy **Logic** Gather worker overload data from the added end point in D76357677 Based on configured policies (cnt of overloaded workers or pct of overloaded workers) and cluster overload, queue the queries **Background** Design doc and rational for the change - https://docs.google.com/document/d/16pEkXPzsP09ZpZ8RxqJ-n-c5kx3TFel0b8Ubx7l6v-I/edit?tab=t.0#heading=h.bdc9ugryon9z Also some follow up questions on the design review / future directions after this change - https://docs.google.com/document/d/16pEkXPzsP09ZpZ8RxqJ-n-c5kx3TFel0b8Ubx7l6v-I/edit?tab=t.0#heading=h.afichcgpu3fe **ODS Metrics on queuing due to this feature:** Right now Queued Queries ODS could be correlated with worker overload to see if this feature is getting activated. Also we log the warn logs when cluster is overloaded. I will add specific metrics for this feature before rollout as well to make debugging easy. **Feature flag:** Right now feature is disabled. We can use coordinator configs to enable / add thresholds Differential Revision: D79470181

facebook-github-bot · 2025-08-06T00:14:44Z

This pull request was exported from Phabricator. Differential Revision: D79470181

…sed on worker load (prestodb#25689) Summary: Pull Request resolved: prestodb#25689 Admission control scheduling policy **Logic** Gather worker overload data from the added end point in PR - prestodb#25687 Based on configured policies (cnt of overloaded workers or pct of overloaded workers) and cluster overload, queue the queries **Background** RFC PR: prestodb/rfcs#42 **Metrics on queuing due to this feature:** Added following JMX metrics - ClusterOverloadDuration - ClusterOverloadCount **Feature flag:** Right now feature is disabled. We can use coordinator configs to enable / add thresholds Differential Revision: D79470181

prashantgolash · 2025-09-08T05:02:09Z

@tdcmeehan - In the RFC we discussed about adding query config to admit DDL, control or light weight queries for cluster level throttling.
I realized that even in current RG framework, we will enqueue those DDL queries.
I was planning to still add config to let cluster checker pass through query based on query types but it would be limited to only checking those query types. (For future extensibility for e.g to admit light/medium weight queries based on cluster load, we might need to move query planning before admission control - where the crux of my logic lies). Please have a look at PR and happy to discuss this further.

Can you add it as a config, similar to per query limits in the resource group config? It's similar--these are global properties, but are tuned on a per session basis, and the per query limits provide a convenient configuration mechanism that allows you to specify the config at the granularity of a resource group.

Done added a RG config to bypass admission throttling.

tdcmeehan · 2025-09-08T14:13:48Z

+
+    public AdmissionControlBypassConfig()
+    {
+        //SelectionCriteria c = new SelectionCriteria(true, "pgolash", Optional.empty(), new HashSet<>(), new ResourceEstimates(Optional.empty(), Optional.empty(), Optional.empty(), Optional.empty()), Optional.of(QueryType.DATA_DEFINITION).map(Enum::name), Optional.empty(), Optional.empty(), Optional.empty());


Remove commented out code.

Please add documentation for new config in https://prestodb.io/docs/current/admin/resource-groups.html#resource-group-properties

tdcmeehan · 2025-09-08T14:19:29Z

                query.setResourceGroupQueryLimits(perQueryLimits);
-                if (canRun && queuedQueries.isEmpty()) {
-                    startInBackground(query);
+                if (shouldBypassAdmissionControl || (canRun && queuedQueries.isEmpty())) {


I don't think it's a good idea to bypass admission control. Instead, just let users configure the resource group to very high values. For example, let the default overload policy be set to a hard count, and let per resource group be the max value for an integer, i.e., set it to an impossibly high number so it effectively doesn't queue.

Can you please explain more. This config is just to bypass the control if the query type in the RG is DDL etc which we discussed. Right now we have cluster level and RG level check in canRunMore. This bypass both of them. I am not sure if I understand about passing very high value as if cluster is loaded it will still get queued.

tdcmeehan · 2025-09-08T14:25:56Z

@sourcery-ai review

sourcery-ai · 2025-09-08T14:28:00Z

Reviewer's Guide

This PR implements coordinator-side query admission control based on worker load. It introduces cluster overload detection with configurable count- or percentage-based policies, integrates throttling into resource groups, and adds a bypass mechanism for special queries. The changes weave the new ClusterResourceChecker and overload policies into InternalResourceGroup and InternalResourceGroupManager, extend node managers to fetch per-node load metrics, and update tests and configuration modules to wire in the new components.

Sequence diagram for query admission control with cluster overload and bypass

sequenceDiagram
    participant Q as actor QuerySubmitter
    participant RG as InternalResourceGroup
    participant CRC as ClusterResourceChecker
    participant ACBC as AdmissionControlBypassConfig
    Q->>RG: run(query)
    RG->>ACBC: shouldBypassAdmissionControl(selectionCriteria)
    ACBC-->>RG: true/false
    alt Admission control bypassed
        RG->>RG: startInBackground(query, true)
    else Admission control required
        RG->>CRC: isClusterCurrentlyOverloaded()
        CRC-->>RG: true/false
        alt Cluster overloaded
            RG->>RG: enqueueQuery(query)
        else Not overloaded
            RG->>RG: startInBackground(query, false)
        end
    end

Class diagram for new and updated cluster overload control classes

classDiagram
    class ClusterResourceChecker {
        - ClusterOverloadPolicy clusterOverloadPolicy
        - ClusterOverloadConfig config
        - InternalNodeManager nodeManager
        + boolean isClusterCurrentlyOverloaded()
        + void addListener(ClusterOverloadStateListener listener)
        + void removeListener(ClusterOverloadStateListener listener)
    }
    class ClusterOverloadPolicy {
        <<interface>>
        + boolean isClusterOverloaded(InternalNodeManager nodeManager)
        + String getName()
    }
    class CpuMemoryOverloadPolicy {
        + boolean isClusterOverloaded(InternalNodeManager nodeManager)
        + String getName()
    }
    class ClusterOverloadPolicyFactory {
        + Optional<ClusterOverloadPolicy> getPolicy(String name)
        + ClusterOverloadPolicy getDefaultPolicy()
        + Map<String, ClusterOverloadPolicy> getPolicies()
    }
    class ClusterOverloadStateListener {
        <<interface>>
        + void onClusterEnteredOverloadedState()
        + void onClusterExitedOverloadedState()
    }
    class ClusterOverloadConfig {
        + boolean isClusterOverloadThrottlingEnabled()
        + double getAllowedOverloadWorkersPct()
        + double getAllowedOverloadWorkersCnt()
        + String getOverloadPolicyType()
    }
    ClusterResourceChecker --> ClusterOverloadPolicy
    ClusterResourceChecker --> ClusterOverloadConfig
    ClusterResourceChecker --> InternalNodeManager
    ClusterResourceChecker --> ClusterOverloadStateListener
    CpuMemoryOverloadPolicy --|> ClusterOverloadPolicy
    ClusterOverloadPolicyFactory --> ClusterOverloadPolicy
    ClusterResourceChecker --> ClusterOverloadPolicyFactory
    ClusterResourceChecker --> ClusterOverloadStateListener
    ClusterOverloadStateListener <|.. InternalResourceGroupManager
    ClusterOverloadStateListener <|.. InternalResourceGroup

Class diagram for resource group admission control and bypass config

classDiagram
    class InternalResourceGroup {
        - AdmissionControlBypassConfig admissionControlPolicyByPassConfig
        - ClusterResourceChecker clusterResourceChecker
        + void setAdmissionControlPolicyByPassConfig(AdmissionControlBypassConfig config)
        + AdmissionControlBypassConfig getAdmissionControlPolicyByPassConfig()
        + void run(ManagedQueryExecution query)
    }
    class AdmissionControlBypassConfig {
        + boolean shouldBypassAdmissionControl(SelectionCriteria selectionCriteria)
        + AdmissionControlBypassConfig setByPassAdmissionControlRules(List<SelectionCriteria> rules)
    }
    class SelectionCriteria {
        + Optional<String> getQueryType()
        // ... other selection fields
    }
    InternalResourceGroup --> AdmissionControlBypassConfig
    AdmissionControlBypassConfig --> SelectionCriteria

Class diagram for node stats and node manager changes

classDiagram
    class InternalNodeManager {
        + Optional<NodeLoadMetrics> getNodeLoadMetrics(String nodeIdentifier)
    }
    class DiscoveryNodeManager {
        - ConcurrentHashMap<String, RemoteNodeStats> nodeStats
        + Optional<NodeLoadMetrics> getNodeLoadMetrics(String nodeId)
    }
    class RemoteNodeStats {
        <<interface>>
        + Optional<NodeStats> getNodeStats()
        + void asyncRefresh()
    }
    class HttpRemoteNodeStats {
        + Optional<NodeStats> getNodeStats()
        + void asyncRefresh()
    }
    class ThriftRemoteNodeStats {
        + Optional<NodeStats> getNodeStats()
        + void asyncRefresh()
    }
    DiscoveryNodeManager --> RemoteNodeStats
    RemoteNodeStats <|.. HttpRemoteNodeStats
    RemoteNodeStats <|.. ThriftRemoteNodeStats
    InternalNodeManager <|.. DiscoveryNodeManager

File-Level Changes

Change	Details	Files
Introduce cluster overload detection and throttling	Define ClusterOverloadConfig with TTL, thresholds, and policy type Implement ClusterResourceChecker (periodic checks, JMX metrics, listener notifications) Add ClusterOverloadPolicy interface, CpuMemoryOverloadPolicy implementation, and factory Wire ClusterResourceChecker into resource group manager and update eligibility on overload exit	`ClusterResourceChecker.java` `ClusterOverloadConfig.java` `CpuMemoryOverloadPolicy.java` `InternalResourceGroup.java` `InternalResourceGroupManager.java` `ServerMainModule.java` `ClusterOverloadPolicyModule.java`
Add admission control bypass rules	Create AdmissionControlBypassConfig and SelectionCriteria in SPI Extend ResourceGroup interface and InternalResourceGroup to store and evaluate bypass config Modify run/startInBackground to skip capacity checks for bypassed queries Add tests for bypass behavior	`AdmissionControlBypassConfig.java` `InternalResourceGroup.java` `ResourceGroup.java` `MockManagedQueryExecution.java` `TestAdmissionControlBypassConfig.java`
Extend node managers to support node load metrics	Rename nodeStates to nodeStats and use RemoteNodeStats instead of RemoteNodeState Introduce HttpRemoteNodeStats and ThriftRemoteNodeStats implementing new RemoteNodeStats Add getNodeLoadMetrics to InternalNodeManager and implementations Refresh node stats asynchronously based on configurable interval	`DiscoveryNodeManager.java` `InternalNodeManager.java` `RemoteNodeStats.java` `HttpRemoteNodeStats.java` `ThriftRemoteNodeStats.java` `InternalCommunicationConfig.java`
Update resource group integration and tests	Inject ClusterResourceChecker into RootInternalResourceGroup and InternalResourceGroupManager constructors Propagate clusterResourceChecker to all test setups Add TestClusterResourceChecker, TestCpuMemoryOverloadPolicy, TestClusterOverloadConfig Extend benchmark and reloading manager tests to include new checker	`TestResourceGroups.java` `TestInternalResourceGroupManager.java` `BenchmarkResourceGroup.java` `TestReloadingResourceGroupConfigurationManager.java` `TestClusterResourceChecker.java` `TestCpuMemoryOverloadPolicy.java` `TestClusterOverloadConfig.java`

Tips and commands

Interacting with Sourcery

Trigger a new review: Comment @sourcery-ai review on the pull request.
Continue discussions: Reply directly to Sourcery's review comments.
Generate a GitHub issue from a review comment: Ask Sourcery to create an
issue from a review comment by replying to it. You can also reply to a
review comment with @sourcery-ai issue to create an issue from it.
Generate a pull request title: Write @sourcery-ai anywhere in the pull
request title to generate a title at any time. You can also comment
@sourcery-ai title on the pull request to (re-)generate the title at any time.
Generate a pull request summary: Write @sourcery-ai summary anywhere in
the pull request body to generate a PR summary at any time exactly where you
want it. You can also comment @sourcery-ai summary on the pull request to
(re-)generate the summary at any time.
Generate reviewer's guide: Comment @sourcery-ai guide on the pull
request to (re-)generate the reviewer's guide at any time.
Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
pull request to resolve all Sourcery comments. Useful if you've already
addressed all the comments and don't want to see them anymore.
Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
request to dismiss all existing Sourcery reviews. Especially useful if you
want to start fresh with a new review - don't forget to comment
@sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

Enable or disable review features such as the Sourcery-generated pull request
summary, the reviewer's guide, and others.
Change the review language.
Add, remove or edit custom review instructions.
Adjust other review settings.

Getting Help

Contact our support team for questions or feedback.
Visit our documentation for detailed guides and information.
Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

sourcery-ai

Hey there - I've reviewed your changes - here's some feedback:

The onClusterEnteredOverloadedState handler is currently empty, so resource groups won’t re-evaluate eligibility when the cluster first becomes overloaded; consider invoking updateEligibilityRecursively there to enforce throttling immediately.
AdmissionControlBypassConfig.shouldBypassAdmissionControl only matches on queryType today but ignores user, source, clientTags, etc.; either extend the matching logic to cover all rule fields or trim the API to only expose supported criteria.
This PR introduces a lot of functionality (cluster overload policies, node stats endpoints, admission bypass, SPI extensions), so you may want to break it into smaller focused changes to simplify review and testing.

Prompt for AI Agents

Please address the comments from this code review:
## Overall Comments
- The onClusterEnteredOverloadedState handler is currently empty, so resource groups won’t re-evaluate eligibility when the cluster first becomes overloaded; consider invoking updateEligibilityRecursively there to enforce throttling immediately.
- AdmissionControlBypassConfig.shouldBypassAdmissionControl only matches on queryType today but ignores user, source, clientTags, etc.; either extend the matching logic to cover all rule fields or trim the API to only expose supported criteria.
- This PR introduces a lot of functionality (cluster overload policies, node stats endpoints, admission bypass, SPI extensions), so you may want to break it into smaller focused changes to simplify review and testing.

## Individual Comments

### Comment 1
<location> `presto-main-base/src/main/java/com/facebook/presto/execution/resourceGroups/InternalResourceGroup.java:828` </location>
<code_context>
     }

-    private void startInBackground(ManagedQueryExecution query)
+    private void startInBackground(ManagedQueryExecution query, boolean shouldBypassAdmissionControl)
     {
         checkState(Thread.holdsLock(root), "Must hold lock to start a query");
         synchronized (root) {
-            runningQueries.add(query);
             InternalResourceGroup group = this;
-            while (group.parent.isPresent()) {
-                group.parent.get().descendantRunningQueries++;
-                group.parent.get().dirtySubGroups.add(group);
-                group = group.parent.get();
+            if (!shouldBypassAdmissionControl) {
+                runningQueries.add(query);
+                while (group.parent.isPresent()) {
</code_context>

<issue_to_address>
startInBackground does not update runningQueries for bypassed queries.

By not updating runningQueries and counters for bypassed queries, resource group statistics and eligibility calculations may become inaccurate. Please review whether bypassed queries should be included in these metrics.
</issue_to_address>

### Comment 2
<location> `presto-main-base/src/test/java/com/facebook/presto/execution/resourceGroups/TestResourceGroups.java` </location>
<code_context>
+    @Test(timeOut = 10_000)
+    public void testAdmissionControlBypass()
</code_context>

<issue_to_address>
Consider adding a test for bypass rules with multiple criteria.

Please add a test case with multiple bypass rules (e.g., DDL and CONTROL) to ensure queries matching any rule are properly bypassed.
</issue_to_address>

### Comment 3
<location> `presto-main-base/src/test/java/com/facebook/presto/execution/resourceGroups/TestResourceGroups.java:1118` </location>
<code_context>
+        root.run(regularQuery2);
+        assertEquals(regularQuery2.getState(), QUEUED);
+
+        // Now run a DDL query - should bypass admission control and run immediately
+        MockManagedQueryExecution ddlQuery = new MockManagedQueryExecution(Optional.of(QueryType.DATA_DEFINITION));
+        ddlQuery.startWaitingForPrerequisites();
+        root.run(ddlQuery);
+        assertEquals(ddlQuery.getState(), RUNNING);
+
+        // Verify we now have 2 running queries (regular + DDL)
</code_context>

<issue_to_address>
Missing negative test for bypass with non-matching query type.

Add a test where a query type not covered by bypass rules is submitted, and confirm it remains subject to admission control.
</issue_to_address>

<suggested_fix>
<<<<<<< SEARCH
        // Now run a DDL query - should bypass admission control and run immediately
        MockManagedQueryExecution ddlQuery = new MockManagedQueryExecution(Optional.of(QueryType.DATA_DEFINITION));
        ddlQuery.startWaitingForPrerequisites();
        root.run(ddlQuery);
        assertEquals(ddlQuery.getState(), RUNNING);

        // Verify we now have 2 running queries (regular + DDL)
        assertEquals(root.getInfo().getNumRunningQueries(), 1);
        assertEquals(root.getInfo().getNumQueuedQueries(), 1);
=======
        // Now run a DDL query - should bypass admission control and run immediately
        MockManagedQueryExecution ddlQuery = new MockManagedQueryExecution(Optional.of(QueryType.DATA_DEFINITION));
        ddlQuery.startWaitingForPrerequisites();
        root.run(ddlQuery);
        assertEquals(ddlQuery.getState(), RUNNING);

        // Negative test: run a query type not covered by bypass rules (e.g., ANALYZE)
        MockManagedQueryExecution analyzeQuery = new MockManagedQueryExecution(Optional.of(QueryType.ANALYZE));
        analyzeQuery.startWaitingForPrerequisites();
        root.run(analyzeQuery);
        // Should be queued, not running
        assertEquals(analyzeQuery.getState(), QUEUED);

        // Verify we now have 2 running queries (regular + DDL) and 2 queued queries (regular2 + analyze)
        assertEquals(root.getInfo().getNumRunningQueries(), 1);
        assertEquals(root.getInfo().getNumQueuedQueries(), 2);
>>>>>>> REPLACE

</suggested_fix>

### Comment 4
<location> `presto-main-base/src/test/java/com/facebook/presto/execution/scheduler/clusterOverload/TestClusterResourceChecker.java` </location>
<code_context>
+    @Test
+    public void testIsClusterCurrentlyOverloaded()
</code_context>

<issue_to_address>
Consider testing listener notification on overload state change.

Add a test that registers a ClusterOverloadStateListener and checks it is notified when the cluster exits the overloaded state to confirm correct listener behavior.
</issue_to_address>

### Comment 5
<location> `presto-main-base/src/test/java/com/facebook/presto/execution/scheduler/clusterOverload/TestCpuMemoryOverloadPolicy.java` </location>
<code_context>
+    @Test
+    public void testIsClusterOverloadedBothMetricsOverloaded()
</code_context>

<issue_to_address>
Consider adding a test for nodes with missing metrics.

Add a test where getNodeLoadMetrics returns Optional.empty() for some nodes, and confirm these nodes are not counted as overloaded by the policy.
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

sourcery-ai · 2025-09-08T14:28:01Z

+            SelectionCriteria selectionCriteria = new SelectionCriteria(query.getSession().getIdentity().getPrincipal().isPresent(), query.getSession().getUser(), query.getSession().getSource(), query.getSession().getClientTags(), query.getSession().getResourceEstimates(), query.getSession().getQueryType().map(Enum::name), query.getSession().getClientInfo(), query.getSession().getSchema(), query.getSession().getIdentity().getPrincipal().map(Principal::getName));
+            boolean shouldBypassAdmissionControl = admissionControlPolicyByPassConfig.shouldBypassAdmissionControl(selectionCriteria);
+
+            if (!shouldBypassAdmissionControl) {


issue (bug_risk): startInBackground does not update runningQueries for bypassed queries.

By not updating runningQueries and counters for bypassed queries, resource group statistics and eligibility calculations may become inaccurate. Please review whether bypassed queries should be included in these metrics.

sourcery-ai · 2025-09-08T14:28:02Z

+        MockManagedQueryExecution ddlQuery = new MockManagedQueryExecution(Optional.of(QueryType.DATA_DEFINITION));
+        ddlQuery.startWaitingForPrerequisites();
+        root.run(ddlQuery);
+        assertEquals(ddlQuery.getState(), RUNNING);


suggestion (testing): Consider adding a test for bypass rules with multiple criteria.

Please add a test case with multiple bypass rules (e.g., DDL and CONTROL) to ensure queries matching any rule are properly bypassed.

sourcery-ai · 2025-09-08T14:28:02Z

+        // Now run a DDL query - should bypass admission control and run immediately
+        MockManagedQueryExecution ddlQuery = new MockManagedQueryExecution(Optional.of(QueryType.DATA_DEFINITION));
+        ddlQuery.startWaitingForPrerequisites();
+        root.run(ddlQuery);
+        assertEquals(ddlQuery.getState(), RUNNING);
+
+        // Verify we now have 2 running queries (regular + DDL)
+        assertEquals(root.getInfo().getNumRunningQueries(), 1);
+        assertEquals(root.getInfo().getNumQueuedQueries(), 1);


suggestion (testing): Missing negative test for bypass with non-matching query type.

Add a test where a query type not covered by bypass rules is submitted, and confirm it remains subject to admission control.

Suggested change

// Now run a DDL query - should bypass admission control and run immediately

MockManagedQueryExecution ddlQuery = new MockManagedQueryExecution(Optional.of(QueryType.DATA_DEFINITION));

ddlQuery.startWaitingForPrerequisites();

root.run(ddlQuery);

assertEquals(ddlQuery.getState(), RUNNING);

// Verify we now have 2 running queries (regular + DDL)

assertEquals(root.getInfo().getNumRunningQueries(), 1);

assertEquals(root.getInfo().getNumQueuedQueries(), 1);

// Now run a DDL query - should bypass admission control and run immediately

MockManagedQueryExecution ddlQuery = new MockManagedQueryExecution(Optional.of(QueryType.DATA_DEFINITION));

ddlQuery.startWaitingForPrerequisites();

root.run(ddlQuery);

assertEquals(ddlQuery.getState(), RUNNING);

// Negative test: run a query type not covered by bypass rules (e.g., ANALYZE)

MockManagedQueryExecution analyzeQuery = new MockManagedQueryExecution(Optional.of(QueryType.ANALYZE));

analyzeQuery.startWaitingForPrerequisites();

root.run(analyzeQuery);

// Should be queued, not running

assertEquals(analyzeQuery.getState(), QUEUED);

// Verify we now have 2 running queries (regular + DDL) and 2 queued queries (regular2 + analyze)

assertEquals(root.getInfo().getNumRunningQueries(), 1);

assertEquals(root.getInfo().getNumQueuedQueries(), 2);

sourcery-ai · 2025-09-08T14:28:02Z

+        assertFalse(clusterResourceChecker.isClusterOverloaded());
+
+        // Stop the periodic task
+        clusterResourceChecker.stop();


suggestion (testing): Consider testing listener notification on overload state change.

Add a test that registers a ClusterOverloadStateListener and checks it is notified when the cluster exits the overloaded state to confirm correct listener behavior.

sourcery-ai · 2025-09-08T14:28:02Z

+
+        // Two nodes are overloaded, but allowed count is 1, so overloaded
+        InternalNodeManager nodeManager = createNodeManager(ImmutableSet.of(createNode("node1", true, false), createNode("node2", false, true), createNode("node3", false, false)));
+        assertTrue(policy.isClusterOverloaded(nodeManager));


suggestion (testing): Consider adding a test for nodes with missing metrics.

Add a test where getNodeLoadMetrics returns Optional.empty() for some nodes, and confirm these nodes are not counted as overloaded by the policy.

…sed on worker load (prestodb#25689) Summary: Pull Request resolved: prestodb#25689 Admission control scheduling policy **Logic** Gather worker overload data from the added end point in PR - prestodb#25687 Based on configured policies (cnt of overloaded workers or pct of overloaded workers) and cluster overload, queue the queries **Background** RFC PR: prestodb/rfcs#42 **Metrics on queuing due to this feature:** Added following JMX metrics - ClusterOverloadDuration - ClusterOverloadCount **Feature flag:** Right now feature is disabled. We can use coordinator configs to enable / add thresholds Differential Revision: D79470181

steveburnett

Thanks for the doc! Some suggestions.

…sed on worker load (prestodb#25689) Summary: Admission control scheduling policy **Logic** Gather worker overload data from the added end point in PR - prestodb#25687 Based on configured policies (cnt of overloaded workers or pct of overloaded workers) and cluster overload, queue the queries **Background** RFC PR: prestodb/rfcs#42 **Metrics on queuing due to this feature:** Added following JMX metrics - ClusterOverloadDuration - ClusterOverloadCount **Feature flag:** Right now feature is disabled. We can use coordinator configs to enable / add thresholds Differential Revision: D79470181

facebook-github-bot · 2025-09-09T00:46:55Z

This pull request was exported from Phabricator. Differential Revision: D79470181

rschlussel

Per offline discussion, the next PR will address adding a configuration for resource groups to admit queries even when the cluster is overloaded #25987

Remaining comments will be addressed as part of #25987

…he worker load metrics (prestodb#25688) Summary: Changes for DiscoveryManager to use the new end point added in the underlying diff Logic 1. Boiler plate code for fetching / refreshing the node stats similar to NodeState -> NodeStats. 2. Consuming NodeState from the new response Once we clean up the NodeState endpoint, we can get rid of `RemoteNodeState` and `HttpRemoteNodeState`. Differential Revision: D76911853

…sed on worker load (prestodb#25689) Summary: Admission control scheduling policy **Logic** Gather worker overload data from the added end point in PR - prestodb#25687 Based on configured policies (cnt of overloaded workers or pct of overloaded workers) and cluster overload, queue the queries **Background** RFC PR: prestodb/rfcs#42 **Metrics on queuing due to this feature:** Added following JMX metrics - ClusterOverloadDuration - ClusterOverloadCount **Feature flag:** Right now feature is disabled. We can use coordinator configs to enable / add thresholds Differential Revision: D79470181

facebook-github-bot · 2025-09-09T18:20:29Z

This pull request was exported from Phabricator. Differential Revision: D79470181

czentgr · 2025-10-02T22:49:52Z

@prashantgolash Can you please add/provide release notes for this PR? Looks like this is something we want to add.
Please see https://github.com/prestodb/presto/wiki/Working-with-the-Presto-release-notes-PR#release-note-entries for guidance. This is usually in the default description but is omitted from this PR.

prashantgolash requested review from a team and shrinidhijoshi as code owners August 5, 2025 06:42

prestodb-ci added the from:Meta PR from Meta label Aug 5, 2025

facebook-github-bot added the fb-exported label Aug 5, 2025

prashantgolash force-pushed the export-D79470181 branch from 8e02c17 to 16d9b55 Compare August 5, 2025 06:44

prashantgolash force-pushed the export-D79470181 branch from 16d9b55 to 79b52b7 Compare August 5, 2025 06:45

prashantgolash changed the title ~~Scheduling Policies for Admission Control based on worker load~~ [Coordinator throttling] Scheduling Policies for Admission Control based on worker load Aug 5, 2025

prashantgolash force-pushed the export-D79470181 branch from 79b52b7 to 841928b Compare August 5, 2025 07:06

prashantgolash force-pushed the export-D79470181 branch from 841928b to af5ae30 Compare August 5, 2025 15:22

prashantgolash force-pushed the export-D79470181 branch from af5ae30 to 5aa384e Compare August 5, 2025 19:37

prashantgolash force-pushed the export-D79470181 branch from 5aa384e to 3c02dea Compare August 6, 2025 00:01

prashantgolash force-pushed the export-D79470181 branch from 3c02dea to add0507 Compare August 6, 2025 00:02

prashantgolash force-pushed the export-D79470181 branch from 8b2f287 to 1ec71e9 Compare September 8, 2025 04:59

rschlussel previously approved these changes Sep 8, 2025

View reviewed changes

tdcmeehan previously requested changes Sep 8, 2025

View reviewed changes

tdcmeehan reviewed Sep 8, 2025

View reviewed changes

sourcery-ai Bot reviewed Sep 8, 2025

View reviewed changes

prashantgolash dismissed rschlussel’s stale review via f3fca15 September 8, 2025 17:35

prashantgolash force-pushed the export-D79470181 branch from 1ec71e9 to f3fca15 Compare September 8, 2025 17:35

prashantgolash requested review from elharo and steveburnett as code owners September 8, 2025 17:35

steveburnett previously requested changes Sep 8, 2025

View reviewed changes

Comment thread presto-docs/src/main/sphinx/admin/resource-groups.rst Outdated

prashantgolash force-pushed the export-D79470181 branch from f3fca15 to a816c3b Compare September 9, 2025 00:46

prashantgolash mentioned this pull request Sep 9, 2025

RG specific config for ClusterOverload admission control #25987

Open

rschlussel approved these changes Sep 9, 2025

View reviewed changes

prashantgolash added 2 commits September 9, 2025 11:18

unidevel mentioned this pull request Sep 11, 2025

Add release notes for 0.295 unix280/presto#42

Closed

13 tasks

This was referenced Sep 24, 2025

Add release notes for 0.295 unix280/presto#43

Closed

Add release notes for 0.295 unix280/presto#45

Merged

unidevel mentioned this pull request Oct 1, 2025

Add release notes for 0.295 unix280/presto#46

Closed

17 tasks

prestodb-ci mentioned this pull request Oct 1, 2025

docs: Add release notes for 0.295 #26208

Merged

17 tasks

Conversation

prashantgolash commented Aug 5, 2025 • edited by sourcery-ai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by Sourcery

Uh oh!

facebook-github-bot commented Aug 5, 2025

Uh oh!

facebook-github-bot commented Aug 5, 2025

Uh oh!

facebook-github-bot commented Aug 5, 2025

Uh oh!

facebook-github-bot commented Aug 5, 2025

Uh oh!

facebook-github-bot commented Aug 5, 2025

Uh oh!

linux-foundation-easycla Bot commented Aug 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tdcmeehan commented Aug 5, 2025

Uh oh!

facebook-github-bot commented Aug 5, 2025

Uh oh!

facebook-github-bot commented Aug 6, 2025

Uh oh!

facebook-github-bot commented Aug 6, 2025

Uh oh!

prashantgolash commented Sep 8, 2025

Uh oh!

tdcmeehan Sep 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tdcmeehan Sep 8, 2025

Choose a reason for hiding this comment

Uh oh!

tdcmeehan Sep 8, 2025

Choose a reason for hiding this comment

Uh oh!

prashantgolash Sep 8, 2025

Choose a reason for hiding this comment

Uh oh!

tdcmeehan commented Sep 8, 2025

Uh oh!

sourcery-ai Bot commented Sep 8, 2025

Reviewer's Guide

Sequence diagram for query admission control with cluster overload and bypass

Class diagram for new and updated cluster overload control classes

Class diagram for resource group admission control and bypass config

Class diagram for node stats and node manager changes

File-Level Changes

Interacting with Sourcery

Customizing Your Experience

Getting Help

Uh oh!

sourcery-ai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

sourcery-ai Bot Sep 8, 2025

Choose a reason for hiding this comment

Uh oh!

sourcery-ai Bot Sep 8, 2025

Choose a reason for hiding this comment

Uh oh!

sourcery-ai Bot Sep 8, 2025

Choose a reason for hiding this comment

Uh oh!

sourcery-ai Bot Sep 8, 2025

Choose a reason for hiding this comment

Uh oh!

sourcery-ai Bot Sep 8, 2025

Choose a reason for hiding this comment

Uh oh!

steveburnett left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

facebook-github-bot commented Sep 9, 2025

Uh oh!

rschlussel left a comment

Choose a reason for hiding this comment

prashantgolash commented Aug 5, 2025 •

edited by sourcery-ai Bot

Loading

linux-foundation-easycla Bot commented Aug 5, 2025 •

edited

Loading

tdcmeehan Sep 8, 2025 •

edited

Loading