feat: complex query thread pool

[Swiddis]

Description

Following discussion on #5608, this PR implements a dedicated thread pool for slow queries. This helps keep the cluster responsive in cases of specific expensive queries being run, as the main worker thread pool will still be available to process fast queries. This comes with a plugins.sql.slow_worker_pool.enabled setting (default true).

The pool exists as a separate resource that the worker can hand off to. So the normal fast query path is unchanged, we only hand off in known special cases. This also means we only pay rescheduling overhead for queries that are already slow.

I've tested this by saturating a cluster with >20sec queries (until all slow threads are active and requests are queued), and then verified that normal cheap queries are still responsive even if slow queries are at capacity.

Guide for reviewers:

• The detection rules live in ScriptDetector.java. We primary look for specific expensive UDFs (rex, parse), window functions, joins, or any script pushdown contexts.
  • The biggest class of false-negatives is aggregations with high cardinality, we can't determine from the query plan alone that an aggregation field is very large. We'd need some sort of statistics tracking on the index. I don't want to put all aggregations in the slow pool since they're very common for dashboarding and many aggregations are low-cardinality.
• Execution dispatch is in ThreadPoolExecutionDispatcher, if we hit the ScriptDetector check we do a handoff involving a lot of thread context propagation and cleanup. This is the riskiest part of the code, I've been carefully checking that the context propagation is working correctly and that we handle things like auth context (field level security) and cancellation.

Related Issues

Resolves #5608

Check List

• New functionality includes testing.
• New functionality has been documented.
• New functionality has javadoc added.
• New functionality has a user manual doc added.
• New PPL command checklist all confirmed.
• API changes companion pull request created.
• Commits are signed per the DCO using --signoff or -s.
• Public documentation issue/PR created.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 737d413)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Possible Issue The cancellation poller schedules a recurring task that interrupts the execution thread when the task is cancelled. However, if the execution completes before the first poll interval (500ms), the poller continues running and may interrupt unrelated work on a reused thread. The poller is cancelled in the finally block, but thread interruption is a one-way signal that persists until explicitly cleared. If the execution thread is returned to the pool with the interrupt flag set, subsequent tasks on that thread may fail spuriously. private Cancellable scheduleCancellationPoller( @Nullable CancellableTask cancellableTask, Thread executionThread) { if (cancellableTask == null) { return NOOP_CANCELLABLE; } return threadPool.scheduleWithFixedDelay( () -> { if (cancellableTask.isCancelled()) { LOG.debug("Task cancelled, interrupting complex pool execution thread"); executionThread.interrupt(); } }, CANCEL_POLL_INTERVAL, ThreadPool.Names.GENERIC); } Possible Issue The timeout handler interrupts the execution thread after the configured timeout. However, the interrupt flag is cleared in the finally block (line 118) only if it was set. If the timeout fires and interrupts the thread, but execution completes normally before the interrupt is processed, the thread is returned to the pool with the interrupt flag set. This can cause subsequent tasks on the reused thread to fail with spurious InterruptedException. threadPool.schedule( () -> { LOG.warn( "Query execution timed out after {}. Interrupting execution thread.", timeout); executionThread.interrupt(); }, timeout, ThreadPool.Names.GENERIC); Possible Issue The optimization step is moved outside the EXECUTING stage and into a separate block before dispatch. If optimization throws an exception, it will not be caught by the EXECUTING stage error handler. This means optimization errors will propagate as unhandled exceptions instead of being wrapped with stage context. The original code optimized inside the EXECUTING stage, ensuring all errors were handled consistently. // Optimize before dispatch so the dispatcher's ScriptDetector // sees the post-optimization plan for accurate routing. RelNode optimizedPlan = CalciteToolsHelper.optimize(calcitePlan, context); analyzeMetric.set(System.nanoTime() - analyzeStart);

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 737d413

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Prevent resource leak on listener exception If failureListener.onFailure(e) throws an exception, the finally block won't execute, leaving the timeout handler and cancel poller active. This causes resource leaks. Wrap the listener callback in a try-catch to ensure cleanup always runs. opensearch/src/main/java/org/opensearch/sql/opensearch/executor/ThreadPoolExecutionDispatcher.java [110-119] } catch (Exception e) { LOG.error("Exception during task execution on complex pool", e); if (failureListener != null) { - failureListener.onFailure(e); + try { + failureListener.onFailure(e); + } catch (Exception listenerException) { + LOG.error("Exception in failure listener", listenerException); + } } } finally { timeoutHandle.cancel(); cancelPoller.cancel(); Thread.interrupted(); Suggestion importance[1-10]: 8 __ Why: This is a valid concern. If failureListener.onFailure(e) throws an exception, the finally block won't execute, causing resource leaks (timeout handler and cancel poller remain active). The suggestion correctly wraps the listener callback to ensure cleanup always runs.	Medium
	Cancel task on timeout The timeout handler interrupts the execution thread but doesn't cancel the underlying task. If the task is blocked on I/O or ignores interrupts, it may continue running indefinitely. Consider also cancelling the cancellableTask when the timeout fires to ensure proper cleanup. opensearch/src/main/java/org/opensearch/sql/opensearch/executor/ThreadPoolExecutionDispatcher.java [83-91] Cancellable timeoutHandle = threadPool.schedule( () -> { LOG.warn( - "Query execution timed out after {}. Interrupting execution thread.", + "Query execution timed out after {}. Interrupting execution thread and cancelling task.", timeout); executionThread.interrupt(); + if (cancellableTask != null) { + cancellableTask.cancel("Query execution timeout"); + } }, timeout, ThreadPool.Names.GENERIC); Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that the timeout handler only interrupts the thread but doesn't cancel the underlying CancellableTask. This could leave tasks running if they ignore interrupts. However, the impact is moderate since the interrupt mechanism is already in place and most tasks should respond to it.	Medium
General	Cancel future on interrupt When InterruptedException is caught, the thread's interrupt flag is restored but the nextBatchFuture may still be running. If the future holds resources or performs I/O, it should be explicitly cancelled to prevent resource leaks and ensure timely cleanup. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/BackgroundSearchScanner.java [122-125] } catch (InterruptedException e) { Thread.currentThread().interrupt(); + nextBatchFuture.cancel(true); throw new TaskCancelledException("The task is cancelled."); } catch (ExecutionException e) { Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies that when InterruptedException is caught, the nextBatchFuture should be cancelled to prevent resource leaks. However, the existing_code snippet doesn't show the full context of where nextBatchFuture is defined, and the improvement assumes it's a Future with a cancel() method, which may not be accurate.	Low
	Fix timing metric on optimization failure If optimization throws an exception, analyzeMetric is set before the exception propagates, recording incorrect timing. The metric should only be set after successful optimization. Move the metric assignment inside a try block or after confirming optimization succeeded. core/src/main/java/org/opensearch/sql/executor/QueryService.java [256-264] -RelNode optimizedPlan = CalciteToolsHelper.optimize(calcitePlan, context); -analyzeMetric.set(System.nanoTime() - analyzeStart); +try { + RelNode optimizedPlan = CalciteToolsHelper.optimize(calcitePlan, context); + analyzeMetric.set(System.nanoTime() - analyzeStart); + + // Wrap execution with EXECUTING stage tracking — dispatch via + // ExecutionDispatcher which may route to a complex worker pool + StageErrorHandler.executeStageVoid( + QueryProcessingStage.EXECUTING, + () -> executionDispatcher.dispatch(optimizedPlan, context, listener, executionEngine), + "while running the query"); +} catch (RuntimeException e) { + // Don't record analyze time on optimization failure + throw e; +} -// Wrap execution with EXECUTING stage tracking — dispatch via -// ExecutionDispatcher which may route to a complex worker pool -StageErrorHandler.executeStageVoid( - QueryProcessingStage.EXECUTING, - () -> executionDispatcher.dispatch(optimizedPlan, context, listener, executionEngine), - "while running the query"); - Suggestion importance[1-10]: 5 __ Why: The suggestion identifies that if optimize() throws an exception, the analyzeMetric is set before the exception propagates. However, the impact is low since this only affects profiling accuracy in error cases, not correctness. The suggested fix adds unnecessary try-catch nesting when the metric could simply be moved after optimization.	Low

---

Previous suggestions

Suggestions up to commit ba6046c

Category	Suggestion	Impact
Possible issue	Cancel task on timeout The timeout handler interrupts the execution thread but doesn't cancel the underlying task. If the task is blocked on I/O or ignores interrupts, it will continue consuming resources. Consider also cancelling the cancellableTask to ensure proper cleanup and prevent resource leaks. opensearch/src/main/java/org/opensearch/sql/opensearch/executor/ThreadPoolExecutionDispatcher.java [82-91] Cancellable timeoutHandle = threadPool.schedule( () -> { LOG.warn( - "Query execution timed out after {}. Interrupting execution thread.", + "Query execution timed out after {}. Interrupting execution thread and cancelling task.", timeout); executionThread.interrupt(); + if (cancellableTask != null) { + cancellableTask.cancel("Query execution timeout"); + } }, timeout, ThreadPool.Names.GENERIC); Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that the timeout handler only interrupts the thread without cancelling the CancellableTask. This could lead to resource leaks if the task ignores interrupts. However, the impact is moderate since thread interruption is already in place.	Medium
General	Cancel future on interrupt When InterruptedException is caught, the thread's interrupt flag is restored but the nextBatchFuture may still be running. This can leave background tasks active after cancellation. Consider cancelling the future before re-interrupting to ensure proper cleanup. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/BackgroundSearchScanner.java [122-125] } catch (InterruptedException e) { + nextBatchFuture.cancel(true); Thread.currentThread().interrupt(); throw new TaskCancelledException("The task is cancelled."); } catch (ExecutionException e) { Suggestion importance[1-10]: 6 __ Why: Valid observation that nextBatchFuture should be cancelled when InterruptedException is caught to prevent background tasks from continuing. This improves cleanup but is not critical since the thread interrupt flag is already restored.	Low
	Cancel task on exception When an exception occurs during task execution, the timeout and cancellation handlers are cancelled in the finally block, but the exception may have left the task in an inconsistent state. If cancellableTask is non-null, explicitly cancel it in the catch block to ensure proper cleanup before the finally block runs. opensearch/src/main/java/org/opensearch/sql/opensearch/executor/ThreadPoolExecutionDispatcher.java [110-115] } catch (Exception e) { LOG.error("Exception during task execution on complex pool", e); + if (cancellableTask != null) { + cancellableTask.cancel("Task execution failed"); + } if (failureListener != null) { failureListener.onFailure(e); } } finally { Suggestion importance[1-10]: 6 __ Why: The suggestion to explicitly cancel cancellableTask on exception is reasonable for ensuring cleanup. However, the finally block already handles cleanup of handlers, and the task cancellation may be redundant depending on the exception type.	Low
	Set metric after optimization If optimization fails with an exception, analyzeMetric is set before the exception propagates, potentially recording incorrect timing. The metric should only be set after successful optimization to ensure accurate profiling data. core/src/main/java/org/opensearch/sql/executor/QueryService.java [256-264] RelNode optimizedPlan = CalciteToolsHelper.optimize(calcitePlan, context); -analyzeMetric.set(System.nanoTime() - analyzeStart); +long optimizeEnd = System.nanoTime(); // Wrap execution with EXECUTING stage tracking — dispatch via // ExecutionDispatcher which may route to a complex worker pool StageErrorHandler.executeStageVoid( QueryProcessingStage.EXECUTING, - () -> executionDispatcher.dispatch(optimizedPlan, context, listener, executionEngine), + () -> { + analyzeMetric.set(optimizeEnd - analyzeStart); + executionDispatcher.dispatch(optimizedPlan, context, listener, executionEngine); + }, "while running the query"); Suggestion importance[1-10]: 5 __ Why: The suggestion identifies a potential timing accuracy issue where analyzeMetric is set before dispatch. However, the current code sets the metric after optimization completes (line 257), so the concern is minor. Moving it inside the lambda would change when it's recorded relative to dispatch overhead.	Low

Suggestions up to commit 1053561

Category	Suggestion	Impact
Possible issue	Cancel future on interrupt When InterruptedException is caught, the thread's interrupt flag is restored but the nextBatchFuture may still be running. The future should be cancelled to prevent resource leaks and ensure the background task stops executing. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/BackgroundSearchScanner.java [122-125] } catch (InterruptedException e) { Thread.currentThread().interrupt(); + nextBatchFuture.cancel(true); throw new TaskCancelledException("The task is cancelled."); } catch (ExecutionException e) { Suggestion importance[1-10]: 8 __ Why: This is a valid resource leak concern. When InterruptedException is caught, the nextBatchFuture should be cancelled to prevent the background task from continuing to execute and consume resources. This is important for proper cleanup.	Medium
	Cancel task on timeout The timeout handler interrupts the execution thread but doesn't cancel the underlying task. If the task is blocked on I/O or ignores interrupts, it will continue running. Consider cancelling the CancellableTask in addition to interrupting the thread to ensure proper cleanup. opensearch/src/main/java/org/opensearch/sql/opensearch/executor/ThreadPoolExecutionDispatcher.java [82-91] Cancellable timeoutHandle = threadPool.schedule( () -> { LOG.warn( - "Query execution timed out after {}. Interrupting execution thread.", + "Query execution timed out after {}. Interrupting execution thread and cancelling task.", timeout); + if (cancellableTask != null) { + cancellableTask.cancel("Query execution timeout"); + } executionThread.interrupt(); }, timeout, ThreadPool.Names.GENERIC); Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that interrupting the thread alone may not stop tasks blocked on I/O. Cancelling the CancellableTask provides a more robust cleanup mechanism. However, the impact is moderate since the interrupt mechanism may already handle most cases.	Medium
General	Fix metric timing accuracy The analyzeMetric is set before dispatching to the complex worker pool. If the dispatcher routes to a different thread, the metric timing won't include the actual execution time. Consider moving the metric recording inside the dispatch callback or after execution completes. core/src/main/java/org/opensearch/sql/executor/QueryService.java [242-250] RelNode optimizedPlan = CalciteToolsHelper.optimize(calcitePlan, context); -analyzeMetric.set(System.nanoTime() - analyzeStart); // Wrap execution with EXECUTING stage tracking — dispatch via // ExecutionDispatcher which may route to a complex worker pool StageErrorHandler.executeStageVoid( QueryProcessingStage.EXECUTING, - () -> executionDispatcher.dispatch(optimizedPlan, context, listener, executionEngine), + () -> { + analyzeMetric.set(System.nanoTime() - analyzeStart); + executionDispatcher.dispatch(optimizedPlan, context, listener, executionEngine); + }, "while running the query"); Suggestion importance[1-10]: 4 __ Why: The suggestion raises a valid point about metric timing, but the current placement appears intentional—the analyzeMetric measures analysis/optimization time, not execution time. Moving it inside dispatch would change what's being measured. The impact is low since this is primarily a profiling concern.	Low
	Check interrupt status before clearing The Thread.interrupted() call clears the interrupt flag but doesn't check its value. If the thread was interrupted due to timeout or cancellation, the exception handler may have already been invoked. Verify the interrupt status before clearing to avoid masking the interrupt signal. opensearch/src/main/java/org/opensearch/sql/opensearch/executor/ThreadPoolExecutionDispatcher.java [110-118] } catch (Exception e) { LOG.error("Exception during task execution on complex pool", e); if (failureListener != null) { failureListener.onFailure(e); } } finally { timeoutHandle.cancel(); cancelPoller.cancel(); - Thread.interrupted(); + boolean wasInterrupted = Thread.interrupted(); + if (wasInterrupted) { + LOG.debug("Execution thread was interrupted, interrupt flag cleared"); + } Suggestion importance[1-10]: 3 __ Why: While checking the interrupt status before clearing provides better observability, the current code correctly clears the flag in the finally block. The suggested logging adds minimal value since interrupts are already handled by the exception handler and timeout/cancellation logic.	Low

Suggestions up to commit aaaa8e4

Category	Suggestion	Impact
Possible issue	Prevent thread leak on listener failure When an exception occurs, the timeout and cancellation polling threads continue running until the finally block executes. If the exception handler itself throws (e.g., failureListener.onFailure fails), these background threads leak. Move cleanup into a nested try-finally to guarantee cancellation. opensearch/src/main/java/org/opensearch/sql/opensearch/executor/ThreadPoolExecutionDispatcher.java [110-118] } catch (Exception e) { LOG.error("Exception during task execution on complex pool", e); - if (failureListener != null) { - failureListener.onFailure(e); + try { + if (failureListener != null) { + failureListener.onFailure(e); + } + } catch (Exception listenerException) { + LOG.error("Exception in failure listener", listenerException); } } finally { timeoutHandle.cancel(); cancelPoller.cancel(); Thread.interrupted(); Suggestion importance[1-10]: 8 __ Why: This is a valid concern about resource leaks. If failureListener.onFailure throws an exception, the finally block won't execute, causing timeoutHandle and cancelPoller to leak. The suggested nested try-catch ensures cleanup happens regardless of listener failures, preventing thread leaks.	Medium
	Cancel task on timeout The timeout handler interrupts the execution thread but doesn't cancel the underlying task. If the task is blocked on I/O or ignores interrupts, it will continue running indefinitely. Consider also cancelling the CancellableTask when the timeout fires to ensure proper cleanup. opensearch/src/main/java/org/opensearch/sql/opensearch/executor/ThreadPoolExecutionDispatcher.java [82-90] Cancellable timeoutHandle = threadPool.schedule( () -> { LOG.warn( - "Query execution timed out after {}. Interrupting execution thread.", + "Query execution timed out after {}. Interrupting execution thread and cancelling task.", timeout); executionThread.interrupt(); + if (cancellableTask != null) { + cancellableTask.cancel("Query execution timed out"); + } }, timeout, ThreadPool.Names.GENERIC); Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that the timeout handler only interrupts the thread but doesn't cancel the underlying CancellableTask. This could lead to tasks continuing to run if they ignore interrupts. However, the impact is moderate since thread interruption is already in place, and the suggestion assumes CancellableTask has a cancel method which needs verification.	Medium
	Cancel future on interrupt When InterruptedException is caught, the thread's interrupt flag is restored but the nextBatchFuture remains active. If the future is blocked on I/O, it won't be cancelled and may hold resources. Cancel the future before rethrowing to ensure proper cleanup. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/BackgroundSearchScanner.java [122-125] } catch (InterruptedException e) { Thread.currentThread().interrupt(); + nextBatchFuture.cancel(true); throw new TaskCancelledException("The task is cancelled."); } catch (ExecutionException e) { Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that when InterruptedException is caught, the nextBatchFuture should be cancelled to release resources. However, the code already restores the interrupt flag with Thread.currentThread().interrupt(), which is good practice. Adding nextBatchFuture.cancel(true) would improve resource cleanup, though the existing code is not critically broken.	Medium
General	Measure analysis time accurately The analyzeMetric is set before dispatching to the complex worker pool, so it measures only the caller thread's work. If optimization is expensive and dispatch adds latency, the metric will be inaccurate. Consider measuring total analysis time including dispatch overhead. core/src/main/java/org/opensearch/sql/executor/QueryService.java [242-250] RelNode optimizedPlan = CalciteToolsHelper.optimize(calcitePlan, context); -analyzeMetric.set(System.nanoTime() - analyzeStart); // Wrap execution with EXECUTING stage tracking — dispatch via // ExecutionDispatcher which may route to a complex worker pool StageErrorHandler.executeStageVoid( QueryProcessingStage.EXECUTING, - () -> executionDispatcher.dispatch(optimizedPlan, context, listener, executionEngine), + () -> { + analyzeMetric.set(System.nanoTime() - analyzeStart); + executionDispatcher.dispatch(optimizedPlan, context, listener, executionEngine); + }, "while running the query"); Suggestion importance[1-10]: 5 __ Why: The suggestion raises a valid point about metric accuracy, but the impact is limited. The current implementation measures analysis time before dispatch, which is reasonable since dispatch overhead is part of execution, not analysis. Moving the metric inside the lambda would include dispatch scheduling time, which may or may not be desired depending on the metric's purpose.	Low

Suggestions up to commit aaaa8e4

Category	Suggestion	Impact
Possible issue	Cancel task on timeout The timeout handler interrupts the execution thread but doesn't cancel the underlying task. If the task is blocked on I/O or ignores interrupts, it will continue running. Consider also cancelling the CancellableTask to ensure proper cleanup and prevent resource leaks. opensearch/src/main/java/org/opensearch/sql/opensearch/executor/ThreadPoolExecutionDispatcher.java [82-91] Cancellable timeoutHandle = threadPool.schedule( () -> { LOG.warn( - "Query execution timed out after {}. Interrupting execution thread.", + "Query execution timed out after {}. Interrupting execution thread and cancelling task.", timeout); + if (cancellableTask != null) { + cancellableTask.cancel("Query execution timed out"); + } executionThread.interrupt(); }, timeout, ThreadPool.Names.GENERIC); Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that the timeout handler only interrupts the thread without cancelling the underlying CancellableTask. This could lead to resource leaks if the task ignores interrupts. However, the impact is moderate since the interrupt mechanism is already in place and may be sufficient for most cases.	Medium
General	Log interrupt status before clearing The finally block clears the interrupt flag with Thread.interrupted() but doesn't check if the thread was actually interrupted. If the thread was interrupted due to timeout or cancellation, this information is lost, potentially masking the root cause of failures. opensearch/src/main/java/org/opensearch/sql/opensearch/executor/ThreadPoolExecutionDispatcher.java [110-118] } catch (Exception e) { LOG.error("Exception during task execution on complex pool", e); if (failureListener != null) { failureListener.onFailure(e); } } finally { timeoutHandle.cancel(); cancelPoller.cancel(); - Thread.interrupted(); + if (Thread.interrupted()) { + LOG.debug("Execution thread was interrupted, likely due to timeout or cancellation"); + } Suggestion importance[1-10]: 4 __ Why: While logging the interrupt status before clearing could provide useful debugging information, the current code already logs errors during execution. The suggestion adds marginal value for debugging but doesn't address a critical issue. The interrupt flag is intentionally cleared to prevent it from affecting subsequent operations.	Low
	Capture optimization time accurately The analyzeMetric is set before dispatching to the execution pool, but if the query is routed to the complex worker pool, there's a delay before actual execution starts. This makes the analyze metric include queuing time, which should be tracked separately for accurate profiling. core/src/main/java/org/opensearch/sql/executor/QueryService.java [242-250] RelNode optimizedPlan = CalciteToolsHelper.optimize(calcitePlan, context); -analyzeMetric.set(System.nanoTime() - analyzeStart); +long optimizeEnd = System.nanoTime(); +analyzeMetric.set(optimizeEnd - analyzeStart); // Wrap execution with EXECUTING stage tracking — dispatch via // ExecutionDispatcher which may route to a complex worker pool StageErrorHandler.executeStageVoid( QueryProcessingStage.EXECUTING, () -> executionDispatcher.dispatch(optimizedPlan, context, listener, executionEngine), "while running the query"); Suggestion importance[1-10]: 3 __ Why: The suggestion adds an intermediate variable optimizeEnd but doesn't change the actual timing calculation. The analyzeMetric is already set immediately after optimization completes (line 243), so the timing is accurate. The suggestion doesn't improve accuracy and only adds an unnecessary variable.	Low

Suggestions up to commit 009300a

Category	Suggestion	Impact
General	Stop cancellation poller after interruption The cancellation poller should stop polling once it detects cancellation to avoid unnecessary overhead. After interrupting the execution thread, the poller continues running until the task completes. Add a check to cancel the scheduled task after interruption. opensearch/src/main/java/org/opensearch/sql/opensearch/executor/ThreadPoolExecutionDispatcher.java [143-157] private Cancellable scheduleCancellationPoller( @Nullable CancellableTask cancellableTask, Thread executionThread) { if (cancellableTask == null) { return NOOP_CANCELLABLE; } - return threadPool.scheduleWithFixedDelay( + final Cancellable[] holder = new Cancellable[1]; + holder[0] = threadPool.scheduleWithFixedDelay( () -> { if (cancellableTask.isCancelled()) { LOG.debug("Task cancelled, interrupting complex pool execution thread"); executionThread.interrupt(); + if (holder[0] != null) { + holder[0].cancel(); + } } }, CANCEL_POLL_INTERVAL, ThreadPool.Names.GENERIC); + return holder[0]; } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that the cancellation poller continues running after interrupting the execution thread, which is inefficient. However, the proposed solution has a potential race condition where holder[0] might be accessed before it's assigned. A better approach would be to return a wrapper Cancellable that cancels itself after interruption.	Medium
	Preserve original exception as cause When catching InterruptedException, the thread's interrupt status is restored but the original exception is lost. The TaskCancelledException should preserve the original cause for better debugging and error tracking. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/BackgroundSearchScanner.java [122-125] } catch (InterruptedException e) { Thread.currentThread().interrupt(); - throw new TaskCancelledException("The task is cancelled."); + throw new TaskCancelledException("The task is cancelled.", e); } catch (ExecutionException e) { Suggestion importance[1-10]: 7 __ Why: Valid suggestion to preserve the original InterruptedException as the cause of TaskCancelledException for better debugging. This improves error tracking without changing the behavior, though the impact is moderate since the exception message already indicates cancellation.	Medium
	Handle InterruptedException explicitly before clearing flag The Thread.interrupted() call clears the interrupt flag but doesn't handle the case where the thread was interrupted during execution. If an InterruptedException occurs, it should be caught separately to ensure proper cleanup and listener notification before clearing the flag. opensearch/src/main/java/org/opensearch/sql/opensearch/executor/ThreadPoolExecutionDispatcher.java [99-107] +} catch (InterruptedException e) { + LOG.error("Task interrupted on complex pool", e); + Thread.currentThread().interrupt(); + if (failureListener != null) { + failureListener.onFailure(e); + } } catch (Exception e) { LOG.error("Exception during task execution on complex pool", e); if (failureListener != null) { failureListener.onFailure(e); } } finally { cancelPoller.cancel(); Thread.interrupted(); Suggestion importance[1-10]: 6 __ Why: The suggestion to handle InterruptedException separately is reasonable for clarity, but the current code already handles interruption correctly by clearing the flag in the finally block. The improvement is marginal since the generic Exception catch already covers InterruptedException and notifies the listener appropriately.	Low
	Cap background pool size limit The background thread pool size is doubled from the search pool size, which may cause resource contention on nodes with many processors. Consider using a fixed multiplier or capping the pool size to prevent excessive thread creation on large nodes. plugin/src/main/java/org/opensearch/sql/plugin/SQLPlugin.java [480-485] new FixedExecutorBuilder( settings, SQL_BACKGROUND_THREAD_POOL_NAME, - settings.getAsInt( - "thread_pool.search.size", 2 * OpenSearchExecutors.allocatedProcessors(settings)), + Math.min( + settings.getAsInt( + "thread_pool.search.size", 2 * OpenSearchExecutors.allocatedProcessors(settings)), + 32), 1000, "thread_pool." + SQL_BACKGROUND_THREAD_POOL_NAME)); Suggestion importance[1-10]: 5 __ Why: The suggestion to cap the background pool size is reasonable to prevent excessive thread creation on large nodes. However, the hardcoded limit of 32 is arbitrary and may not be appropriate for all deployments. A configurable setting or a more principled calculation would be better. The current implementation already uses 2 * allocatedProcessors, which is a reasonable default.	Low

[github-actions]

Persistent review updated to latest commit 46c3cbe

[github-actions]

Persistent review updated to latest commit 6fdd82e

[github-actions]

Persistent review updated to latest commit 434021f

[github-actions]

Persistent review updated to latest commit 559a2d0

[github-actions]

Persistent review updated to latest commit 559a2d0

[github-actions]

Persistent review updated to latest commit 73c68cf

[github-actions]

Persistent review updated to latest commit 21dd714

[github-actions]

Persistent review updated to latest commit 9ba8eef

[github-actions]

Persistent review updated to latest commit 009300a

[github-actions]

Persistent review updated to latest commit 7580ddb

[github-actions]

Persistent review updated to latest commit aaaa8e4

[github-actions]

Persistent review updated to latest commit 1053561

[github-actions]

Persistent review updated to latest commit ba6046c

[github-actions]

Persistent review updated to latest commit 737d413