CAMEL-24272: Migrate reconnection loops from ForegroundTask to BackgroundTask - #25164
Conversation
…oundTask Switch all reconnection and retry loops from ForegroundTask to BackgroundTask so they register with the TaskManagerRegistry and become visible in the dev console, CLI and TUI internal-tasks view. Affected components: - camel-kafka: KafkaConsumer create/subscribe retry loops - camel-ftp: FTP and SFTP reconnection loops - camel-mllp: TCP server bind retry loop - camel-infinispan: Schema registration retry loop - camel-google-pubsub: Error recovery delay - camel-zookeeper: Reconnection backoff delay - camel-hazelcast: SEDA error recovery delay - camel-salesforce: Handshake and subscribe retry delays - camel-mongodb-gridfs: GridFS polling loop Each migration: - Replaces Tasks.foregroundTask() with Tasks.backgroundTask() - Switches from IterationBudget to TimeBudget via iterationTimeBudget() - Adds a ScheduledExecutorService with proper lifecycle management - Assigns a descriptive task name for visibility in monitoring tools Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 23 tested, 29 compile-only — current: 23 all testedMaveniverse Scalpel detected 52 affected modules (current approach: 23).
|
…asks ScheduledExecutorService.scheduleWithFixedDelay() rejects delay=0 with IllegalArgumentException. Change interval from Duration.ZERO to Duration.ofMillis(1) for single-iteration delay tasks where the interval is irrelevant (maxIterations=1). Affected: camel-hazelcast, camel-zookeeper, camel-google-pubsub, camel-salesforce. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Upstream already migrated KafkaFetchRecords to BackgroundTask with a unified reconnectTask() pattern. Taking upstream's version since it supersedes our separate create/subscribe task approach. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The test expectations were not updated when the TypeConvertersTab and TransformersTab were added on main. Update count from 29 to 31 and add the Y and F shortcut letters to the historical sequence. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
davsclaus
left a comment
There was a problem hiding this comment.
Thanks for this well-structured migration, Guillaume! The changes look good — the ForegroundTask-to-BackgroundTask migration is mechanically sound across all 9 components, executor lifecycle management is correct, and CI passes.
A couple of minor observations noted inline — nothing blocking.
This review does not replace specialized static analysis tools (SonarCloud) or AI review tools (CodeRabbit).
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of davsclaus
| } | ||
|
|
||
| executor = endpoint.createExecutor(this); | ||
| taskExecutor = endpoint.getCamelContext().getExecutorServiceManager() |
There was a problem hiding this comment.
Minor observation: this creates a single-thread scheduled executor shared across all concurrentConsumers threads. When multiple subscribers fail simultaneously, their backoff delays are serialized rather than independent (unlike the prior ForegroundTask which slept per-thread).
In practice the impact is negligible since the delay supplier is () -> true (near-instant execution), but worth noting the behavioral difference. The same applies to HazelcastSedaConsumer.
There was a problem hiding this comment.
Good observation. The single-thread executor does serialize backoff delays across concurrent subscribers. However, this is a single-iteration delay task (maxIterations=1) used only as a timed pause before retry — it completes almost instantly once the initial delay expires. With a 10-second reconnect delay, even if multiple subscribers fail simultaneously, the serialization overhead is negligible compared to the delay itself.
That said, if this becomes a concern in practice (e.g. very high concurrency with frequent failures), the executor pool size could be bumped. For now, keeping it single-threaded matches the pattern used by the other components in this PR.
| .withMaxIterations(IterationBoundedBudget.UNLIMITED_ITERATIONS) | ||
| BlockingTask task = Tasks.backgroundTask() | ||
| .withBudget(Budgets.iterationTimeBudget() | ||
| .withMaxIterations(Integer.MAX_VALUE) |
There was a problem hiding this comment.
Nit: Integer.MAX_VALUE is already the default for IterationTimeBoundedBudgetBuilder.maxIterations, so this call could be omitted entirely:
| .withMaxIterations(Integer.MAX_VALUE) | |
| // maxIterations defaults to Integer.MAX_VALUE (effectively unlimited) |
Alternatively, keeping it explicit is fine for readability — just noting the redundancy.
There was a problem hiding this comment.
Good catch — removed the redundant withMaxIterations(Integer.MAX_VALUE) and added a comment noting the default. Fixed in the next push.
… GridFsConsumer Integer.MAX_VALUE is already the default for IterationTimeBoundedBudgetBuilder, so the explicit call is unnecessary. Added a comment noting the default. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
LGTM |
Summary
Claude Code on behalf of gnodet
Migrate reconnection loops across 9 components from
ForegroundTasktoBackgroundTaskso they register withTaskManagerRegistryand become visible in the TUI/CLI/dev console internal task view.Note: The Kafka component (
KafkaFetchRecords.java) was already independently migrated toBackgroundTaskupstream, so this PR now covers the remaining 9 components.Components migrated
FtpOperations.javaSftpOperations.javaTcpServerBindThread.javaInfinispanRemoteManager.javaGooglePubsubConsumer.javaZooKeeperConsumer.javaHazelcastSedaConsumer.javaSubscriptionHelper.javaGridFsConsumer.javaKey changes
ForegroundTask→BackgroundTask: Each migrated task now registers withTaskManagerRegistry, making internal retry/reconnect tasks visible through management interfacesIterationBudget→IterationTimeBudget:BackgroundTaskrequires aTimeBudget; usedwithUnlimitedDuration()to preserve existing behaviorScheduledExecutorService: Each component gets a dedicated scheduled executor for task scheduling, managed through Camel'sExecutorServiceManagerfor proper lifecyclemaxIterations(1)as a delay mechanism, interval is set toDuration.ofMillis(1)sinceScheduledExecutorService.scheduleWithFixedDelay()rejectsdelay=0Test plan
HazelcastSedaRecoverableConsumerNewTransactionTestandHazelcastSedaRecoverableConsumerRollbackTestverified locally🤖 Generated with Claude Code