- 
                Notifications
    
You must be signed in to change notification settings  - Fork 25.6k
 
ESQL: Calculate concurrent node limit #124901
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
      
      
            idegtiarenko
  merged 28 commits into
  elastic:main
from
ivancea:esql-calculate-concurrent-requests-limit
  
      
      
   
  Mar 25, 2025 
      
    
  
     Merged
                    Changes from all commits
      Commits
    
    
            Show all changes
          
          
            28 commits
          
        
        Select commit
          Hold shift + click to select a range
      
      eec2039
              
                Limit concurrent node requests
              
              
                idegtiarenko d4850fc
              
                upd
              
              
                idegtiarenko ae96763
              
                upd
              
              
                idegtiarenko 15d897f
              
                Merge branch 'main' into limit_concurrent_node_requests
              
              
                idegtiarenko 666f588
              
                do not send requests if source is completed
              
              
                idegtiarenko 34badf7
              
                Merge branch 'main' into limit_concurrent_node_requests
              
              
                idegtiarenko 8c55e8a
              
                upd
              
              
                idegtiarenko b2a66a2
              
                do not erase prior shard failures on skipping node
              
              
                idegtiarenko 11bd0f0
              
                Merge branch 'main' into limit_concurrent_node_requests
              
              
                idegtiarenko 53f7d60
              
                upd
              
              
                idegtiarenko 3fe1766
              
                Initial calculator with tests
              
              
                ivancea d4fcb2e
              
                Merge branch 'main' into esql-calculate-concurrent-requests-limit
              
              
                ivancea a8faf76
              
                Fix NPE
              
              
                ivancea 97cf29e
              
                [CI] Auto commit changes from spotless
              
              
                 a5af34c
              
                Fix test by optimizing plans
              
              
                ivancea 6e41a98
              
                Merge branch 'main' into esql-calculate-concurrent-requests-limit
              
              
                ivancea 4fb514e
              
                Added whitelisted nodes, removed multiple Limits assertion, and avoid…
              
              
                ivancea 5acabda
              
                [CI] Auto commit changes from spotless
              
              
                 bc4be1d
              
                Removed node whitelist, as limit is pushed down, and update concurren…
              
              
                ivancea 36c05ee
              
                Remove limit from Pragma
              
              
                ivancea 7fa6f18
              
                Merge branch 'main' into esql-calculate-concurrent-requests-limit
              
              
                ivancea 6bf6007
              
                Add Nullable annotations, and cleanup
              
              
                ivancea a37ca1f
              
                Avoid limiting on high limits
              
              
                ivancea 6ea6df5
              
                Update docs/changelog/124901.yaml
              
              
                ivancea cdb007e
              
                Merge branch 'main' into esql-calculate-concurrent-requests-limit
              
              
                ivancea 44e6399
              
                Fix tests after max limit logic
              
              
                ivancea 6cc0f42
              
                fix comments
              
              
                idegtiarenko 9f930b6
              
                Merge branch 'main' into esql-calculate-concurrent-requests-limit
              
              
                idegtiarenko File filter
Filter by extension
Conversations
          Failed to load comments.   
        
        
          
      Loading
        
  Jump to
        
          Jump to file
        
      
      
          Failed to load files.   
        
        
          
      Loading
        
  Diff view
Diff view
There are no files selected for viewing
  
    
      This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
      Learn more about bidirectional Unicode characters
    
  
  
    
              | Original file line number | Diff line number | Diff line change | 
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| pr: 124901 | ||
| summary: Calculate concurrent node limit | ||
| area: ES|QL | ||
| type: feature | ||
| issues: [] | 
        
          
          
            106 changes: 106 additions & 0 deletions
          
          106 
        
  ...in/esql/src/main/java/org/elasticsearch/xpack/esql/planner/PlanConcurrencyCalculator.java
  
  
      
      
   
        
      
      
    
  
    
      This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
      Learn more about bidirectional Unicode characters
    
  
  
    
              | Original file line number | Diff line number | Diff line change | 
|---|---|---|
| @@ -0,0 +1,106 @@ | ||
| /* | ||
| * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
| * or more contributor license agreements. Licensed under the Elastic License | ||
| * 2.0; you may not use this file except in compliance with the Elastic License | ||
| * 2.0. | ||
| */ | ||
| 
     | 
||
| package org.elasticsearch.xpack.esql.planner; | ||
| 
     | 
||
| import org.elasticsearch.core.Nullable; | ||
| import org.elasticsearch.xpack.esql.core.expression.Literal; | ||
| import org.elasticsearch.xpack.esql.core.util.Holder; | ||
| import org.elasticsearch.xpack.esql.plan.logical.EsRelation; | ||
| import org.elasticsearch.xpack.esql.plan.logical.Limit; | ||
| import org.elasticsearch.xpack.esql.plan.logical.LogicalPlan; | ||
| import org.elasticsearch.xpack.esql.plan.physical.FragmentExec; | ||
| import org.elasticsearch.xpack.esql.plan.physical.PhysicalPlan; | ||
| import org.elasticsearch.xpack.esql.session.Configuration; | ||
| 
     | 
||
| /** | ||
| * Calculates the maximum number of nodes that should be queried concurrently for the given data node plan. | ||
| * <p> | ||
| * Used to avoid overloading the cluster with concurrent requests that may not be needed. | ||
| * </p> | ||
| */ | ||
| public class PlanConcurrencyCalculator { | ||
| public static final PlanConcurrencyCalculator INSTANCE = new PlanConcurrencyCalculator(); | ||
| 
     | 
||
| private PlanConcurrencyCalculator() {} | ||
| 
     | 
||
| /** | ||
| * @return {@code null} if there should be no limit, otherwise, the maximum number of nodes that should be queried concurrently. | ||
| */ | ||
| @Nullable | ||
| public Integer calculateNodesConcurrency(PhysicalPlan dataNodePlan, Configuration configuration) { | ||
| // If available, pragma overrides any calculation | ||
| if (configuration.pragmas().maxConcurrentNodesPerCluster() > 0) { | ||
| return configuration.pragmas().maxConcurrentNodesPerCluster(); | ||
| } | ||
| if (dataNodePlan == null) { | ||
| return null; | ||
| } | ||
| 
     | 
||
| Integer dataNodeLimit = getDataNodeLimit(dataNodePlan); | ||
| 
     | 
||
| if (dataNodeLimit != null) { | ||
| return limitToConcurrency(dataNodeLimit); | ||
| } | ||
| 
     | 
||
| return null; | ||
| } | ||
| 
     | 
||
| private Integer limitToConcurrency(int limit) { | ||
| // For high limits, don't limit the concurrency | ||
| if (limit > 1000) { | ||
| return null; | ||
| } | ||
| 
     | 
||
| // At least 2 nodes, otherwise log2(limit). E.g. | ||
| // Limit | Concurrency | ||
| // 1 | 2 | ||
| // 10 | 3 | ||
| // 1000 | 9 | ||
| return Math.max(2, (int) (Math.log(limit) / Math.log(2))); | ||
| 
         There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Super-duper driveby: maybe it's simpler to take   | 
||
| } | ||
| 
     | 
||
| @Nullable | ||
| private Integer getDataNodeLimit(PhysicalPlan dataNodePlan) { | ||
| LogicalPlan logicalPlan = getFragmentPlan(dataNodePlan); | ||
| 
     | 
||
| // State machine to find: | ||
| // A relation | ||
| Holder<Boolean> relationFound = new Holder<>(false); | ||
| // ...followed by no other node that could break the calculation | ||
| Holder<Boolean> forbiddenNodeFound = new Holder<>(false); | ||
| // ...and finally, a limit | ||
| Holder<Integer> limitValue = new Holder<>(null); | ||
| 
     | 
||
| logicalPlan.forEachUp(node -> { | ||
| // If a limit or a forbidden command was already found, ignore the rest | ||
| if (limitValue.get() == null && forbiddenNodeFound.get() == false) { | ||
| if (node instanceof EsRelation) { | ||
| relationFound.set(true); | ||
| } else if (relationFound.get()) { | ||
| if (node instanceof Limit limit && limit.limit() instanceof Literal literalLimit) { | ||
| limitValue.set((Integer) literalLimit.value()); | ||
| } else { | ||
| forbiddenNodeFound.set(true); | ||
| } | ||
| } | ||
| } | ||
| }); | ||
| 
     | 
||
| return limitValue.get(); | ||
| } | ||
| 
     | 
||
| private LogicalPlan getFragmentPlan(PhysicalPlan plan) { | ||
| Holder<LogicalPlan> foundPlan = new Holder<>(); | ||
| plan.forEachDown(node -> { | ||
| if (node instanceof FragmentExec fragment) { | ||
| foundPlan.set(fragment.fragment()); | ||
| } | ||
| }); | ||
| return foundPlan.get(); | ||
| } | ||
| } | ||
  
    
      This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
      Learn more about bidirectional Unicode characters
    
  
  
    
              
  
    
      This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
      Learn more about bidirectional Unicode characters
    
  
  
    
              
      
      Oops, something went wrong.
        
    
  
      
      Oops, something went wrong.
        
    
  
  Add this suggestion to a batch that can be applied as a single commit.
  This suggestion is invalid because no changes were made to the code.
  Suggestions cannot be applied while the pull request is closed.
  Suggestions cannot be applied while viewing a subset of changes.
  Only one suggestion per line can be applied in a batch.
  Add this suggestion to a batch that can be applied as a single commit.
  Applying suggestions on deleted lines is not supported.
  You must change the existing code in this line in order to create a valid suggestion.
  Outdated suggestions cannot be applied.
  This suggestion has been applied or marked resolved.
  Suggestions cannot be applied from pending reviews.
  Suggestions cannot be applied on multi-line comments.
  Suggestions cannot be applied while the pull request is queued to merge.
  Suggestion cannot be applied right now. Please check back later.
  
    
  
    
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets not limit queries with limits higher than 1000 for now.
It might become slower when querying a lot of shards with small number of shards.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Above makes sense, but I would like to confirm with @costin about it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with this heuristic. You can always override it.
Do we get here with
| LIMIT 0? Could you make sure we have tests for that?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added