Conversation
…mers PollingEventSource and TimerEventSource each created a java.util.Timer, and PerResourcePollingEventSource a ScheduledThreadPoolExecutor of its own, so every polling event source and every controller cost a thread that the operator neither sized nor shut down. They now schedule on executors managed by ExecutorServiceManager, which builds them from ConfigurationService: - getScheduledExecutorService() backs the polling event sources - getRetryAndRescheduleExecutorService() backs the retry and reschedule timer of every controller, kept separate so that a slow poll cannot delay a retry Both are sized by concurrentScheduledTaskThreads() (4 by default) and concurrentRetryAndRescheduleThreads() (2 by default), overridable programmatically or through the josdk.scheduled-tasks.concurrent-threads and josdk.retry-and-reschedule.concurrent-threads keys. Both default to daemon threads, discard the tasks scheduled for later on shutdown so they don't hold up the termination of the operator, and drop cancelled tasks eagerly. The previous Executors.newScheduledThreadPool(0) was effectively single threaded and created non daemon threads. An event source resolves its executor on every start rather than at creation time, since the manager replaces its pools when the operator is restarted, and only shuts one down if it created it itself. That also fixes PerResourcePollingEventSource shutting down a user supplied executor and not registering its tasks again after a restart. PollingEventSource gains a constructor taking an EventSourceContext, the one without it is deprecated, and both polling configurations accept an executor to run a single event source on a pool of its own.
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
csviri
marked this pull request as draft
September 17, 2026 07:45
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Retain the removed public constant and prevent late polling tasks from re-registering after shutdown.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Refactors polling, retry, and rescheduling onto configurable operator-managed executors, with lifecycle improvements, tests, and documentation.
Changes:
- Adds configurable scheduled-task and retry/reschedule executor pools.
- Updates timer and polling event sources for shared or custom executors.
- Adds configuration, lifecycle tests, and documentation.
- Two critical issues remain unresolved: API compatibility and a polling restart race.
File summaries
| File | Description |
|---|---|
operator-framework/src/test/java/io/javaoperatorsdk/operator/dependent/multiplemanagedexternaldependenttype/MultipleManagedExternalDependentResourceReconciler.java |
Updates test construction for the context-aware API. |
operator-framework/src/test/java/io/javaoperatorsdk/operator/config/loader/ConfigLoaderTest.java |
Tests executor configuration loading. |
operator-framework/src/main/java/io/javaoperatorsdk/operator/config/loader/ConfigLoader.java |
Loads executor thread settings. |
operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/timer/TimerEventSourceTest.java |
Tests timer executor behavior and lifecycle. |
operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/polling/PollingEventSourceTest.java |
Tests polling cancellation and shared scheduling. |
operator-framework-core/src/test/java/io/javaoperatorsdk/operator/api/config/ExecutorServiceManagerTest.java |
Tests executor lifecycle management. |
operator-framework-core/src/test/java/io/javaoperatorsdk/operator/api/config/ConfigurationServiceOverriderTest.java |
Tests executor overrides and defaults. |
operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/timer/TimerEventSource.java |
Replaces timers with scheduled executors. |
operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/polling/PollingEventSource.java |
Supports managed and custom polling executors. |
operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/polling/PollingConfigurationBuilder.java |
Adds polling executor configuration. |
operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/polling/PollingConfiguration.java |
Stores polling executor configuration. |
operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/polling/PerResourcePollingEventSource.java |
Migrates per-resource polling to managed executors. |
operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/polling/PerResourcePollingConfiguration.java |
Updates per-resource executor configuration. |
operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/EventSources.java |
Injects retry executor suppliers. |
operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/EventSourceManager.java |
Connects controllers to managed retry executors. |
operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/external/PollingDependentResource.java |
Passes event-source context to polling sources. |
operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/Utils.java |
Provides daemon scheduled-executor utilities. |
operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ExecutorServiceManager.java |
Manages scheduled executor lifecycles. |
operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationServiceOverrider.java |
Supports executor customization. |
operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java |
Defines executor defaults and factories. |
docs/content/en/docs/documentation/operations/configuration.md |
Documents executor configuration keys. |
docs/content/en/docs/documentation/eventing.md |
Documents polling and retry executor behavior. |
Review details
- Files reviewed: 22/22 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| executorService == null | ||
| ? new ScheduledThreadPoolExecutor(DEFAULT_EXECUTOR_THREAD_NUMBER) | ||
| : executorService; | ||
| this.executorService = executorService; |
Comment on lines
+250
to
+253
| // the tasks have to be cancelled explicitly now that the executor can be shared with the rest | ||
| // of the operator, and the map cleared so that they are registered again on a restart | ||
| scheduledFutures.values().forEach(future -> future.cancel(true)); | ||
| scheduledFutures.clear(); |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
PollingEventSource and TimerEventSource each created a java.util.Timer,
and PerResourcePollingEventSource a ScheduledThreadPoolExecutor of its
own, so every polling event source and every controller cost a thread
that the operator neither sized nor shut down.
They now schedule on executors managed by ExecutorServiceManager, which
builds them from ConfigurationService:
timer of every controller, kept separate so that a slow poll cannot
delay a retry
Both are sized by concurrentScheduledTaskThreads() (4 by default) and
concurrentRetryAndRescheduleThreads() (2 by default), overridable
programmatically or through the josdk.scheduled-tasks.concurrent-threads
and josdk.retry-and-reschedule.concurrent-threads keys. Both default to
daemon threads, discard the tasks scheduled for later on shutdown so
they don't hold up the termination of the operator, and drop cancelled
tasks eagerly. The previous Executors.newScheduledThreadPool(0) was
effectively single threaded and created non daemon threads.
An event source resolves its executor on every start rather than at
creation time, since the manager replaces its pools when the operator is
restarted, and only shuts one down if it created it itself. That also
fixes PerResourcePollingEventSource shutting down a user supplied
executor and not registering its tasks again after a restart.
PollingEventSource gains a constructor taking an EventSourceContext, the
one without it is deprecated, and both polling configurations accept an
executor to run a single event source on a pool of its own.