CAMEL-23352: Add syncOptimisticRetry option to Aggregate EIP by code-massel · Pull Request #22769 · apache/camel

code-massel · 2026-04-24T21:36:34Z

When optimisticLocking is enabled, retries are normally scheduled asynchronously on a background ScheduledExecutorService, which switches threads and breaks transactional use cases where the aggregation repository and inbound message store must commit/rollback within a single transaction boundary.

The new syncOptimisticRetry flag (default false) causes the retry delay to execute via Thread.sleep on the calling thread instead, preserving the transaction context. This restores the Camel 2.x behavior for users who need it, without changing the default async behavior.

Description

When optimisticLocking is enabled on the Aggregate EIP, retry failures are scheduled asynchronously on a background ScheduledExecutorService. This thread switch breaks transactional use cases where the aggregation repository and inbound message store must commit/rollback within a single transaction boundary (transaction context is bound to the originating thread).

This PR adds a new syncOptimisticRetry option (default false) to the aggregate EIP. When enabled, the retry delay runs via Thread.sleep on the calling thread instead of scheduling on a background executor, preserving the transaction context. Default is false so existing routes are unaffected.

Target

I checked that the commit is targeting the correct branch (Camel 4 uses the main branch)

Tracking

If this is a large change, bug fix, or code improvement, I checked there is a https://issues.apache.org/jira/browse/CAMEL-23352 filed for the change.

Apache Camel coding standards and style

I checked that each commit in the pull request has a meaningful subject line and body.
I have run mvn clean install -DskipTests locally from root folder and I have committed all auto-generated changes.

github-actions · 2026-04-25T06:55:01Z

🌟 Thank you for your contribution to the Apache Camel project! 🌟
🤖 CI automation will test this PR automatically.

🐫 Apache Camel Committers, please review the following items:

First-time contributors require MANUAL approval for the GitHub Actions to run
You can use the command /component-test (camel-)component-name1 (camel-)component-name2.. to request a test from the test bot although they are normally detected and executed by CI.
You can label PRs using skip-tests and test-dependents to fine-tune the checks executed by this PR.
Build and test logs are available in the summary page. Only Apache Camel committers have access to the summary.

⚠️ Be careful when sharing logs. Review their contents before sharing them publicly.

davsclaus · 2026-04-25T08:09:02Z

when you add new options to EIPs you generally need to rebuild the entire code base

	modified:   catalog/camel-catalog/src/generated/resources/org/apache/camel/catalog/models/aggregate.json
	modified:   catalog/camel-catalog/src/generated/resources/org/apache/camel/catalog/schemas/camel-spring.xsd
	modified:   catalog/camel-catalog/src/generated/resources/org/apache/camel/catalog/schemas/camel-xml-io.xsd
	modified:   core/camel-core-model/src/generated/resources/META-INF/org/apache/camel/model/aggregate.json
	modified:   core/camel-xml-io/src/generated/java/org/apache/camel/xml/in/ModelParser.java
	modified:   core/camel-xml-io/src/generated/java/org/apache/camel/xml/out/ModelWriter.java
	modified:   core/camel-yaml-io/src/generated/java/org/apache/camel/yaml/out/ModelWriter.java
	modified:   dsl/camel-yaml-dsl/camel-yaml-dsl-deserializers/src/generated/java/org/apache/camel/dsl/yaml/deserializers/ModelDeserializers.java
	modified:   dsl/camel-yaml-dsl/camel-yaml-dsl/src/generated/resources/schema/camelYamlDsl.json

davsclaus · 2026-04-25T08:09:41Z

for example run from root

mvn clean install -P fastinstall

gnodet

Review: CAMEL-23352 — Add `syncOptimisticRetry` option to Aggregate EIP

Thank you for this contribution! The use case is well-motivated — async optimistic locking retries breaking transaction context is a real problem, and adding an opt-in synchronous retry is a clean approach. The core implementation logic is correct.

A few items need attention before this can merge.

Blocker: Missing generated files

The PR modifies 4 hand-written files but does not include any regenerated downstream artifacts. CI will fail at the "Fail if there are uncommitted changes" step. Please run:

mvn install -B -pl core/camel-core-model -DskipTests

Then check git status and commit all generated file changes. This will update at minimum:

core/camel-core-model/src/generated/ — JSON model metadata, configurers
dsl/camel-yaml-dsl/ — YAML DSL schema and deserializers

The auto-generated options table in the aggregate EIP docs (aggregate-eip.adoc) will also pick up the new option from the Javadoc after regeneration, so a separate prose change is likely not needed.

Implementation

The core retry logic is sound:

The syncOptimisticRetry branch correctly calls doDelay(attempt) then continues the retry loop on the calling thread.
InterruptedException handling correctly restores the interrupt flag and propagates via exchange.setException().
Default (false) preserves existing async behavior — no regression risk.
The reifier and copy constructor correctly wire the new option.

See inline comments for minor suggestions.

Claude Code on behalf of Guillaume Nodet

code-massel · 2026-05-05T20:18:24Z

Thanks for the review! I've addressed your feedback in the latest push:

Renamed syncOptimisticRetry → optimisticLockingSyncRetry across all files for consistency with the existing optimisticLocking option naming
Updated Javadoc to clarify the flag only takes effect when optimisticLocking() is also enabled
Added two new tests: one verifying synchronous retry with a non-zero delay stays on the calling thread, and one exercising the InterruptedException path when the thread is interrupted mid-retry
Regenerated all downstream artifacts (catalog, YAML DSL, XML IO, XSD schemas)

Let me know if there's anything else to adjust

Croway · 2026-05-06T08:25:56Z

+        sender.start();
+
+        // Let at least one retry attempt happen before interrupting
+        Thread.sleep(100);


Can you use Awaitility instead of this Thread.sleep?

Croway · 2026-05-06T08:30:16Z

There are some uncommitted changes:

Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
	modified:   catalog/camel-catalog/src/generated/resources/org/apache/camel/catalog/schemas/camel-spring.xsd
	modified:   catalog/camel-catalog/src/generated/resources/org/apache/camel/catalog/schemas/camel-xml-io.xsd
	modified:   dsl/camel-yaml-dsl/camel-yaml-dsl/src/generated/resources/schema/camelYamlDsl-canonical.json
	```

davsclaus · 2026-05-07T07:12:25Z

@code-massel can you do a full rebuild and add the modified files to this PR

mvn clean install -P fastinstall

from the root folder

gnodet

Second Review: CAMEL-23352 — `optimisticLockingSyncRetry` option for Aggregate EIP

Claude Code on behalf of Guillaume Nodet

Thank you for addressing the first round of feedback — the rename to optimisticLockingSyncRetry, the improved Javadoc, and the two additional tests (nonzero delay + interrupt path) are all solid improvements. The core implementation in AggregateProcessor.doInOptimisticLock() looks correct: the synchronous doDelay() + continue path cleanly preserves the calling thread, and the InterruptedException handling is done properly with Thread.currentThread().interrupt().

This second round focuses on the test class, specifically around style conventions and test robustness.

Summary of Findings

| # | Severity | Finding |
|---|----------|---------||
| 1 | Must fix | Thread.sleep(100) in testSyncRetryInterrupted — use Awaitility (project rule, also flagged by @Croway) |
| 2 | Must fix | Conditional assertions silently pass — if (aggregateThreadName != null) and if (result != null && ...) mean the test can succeed without ever verifying the core behavior |
| 3 | Should fix | Test class and methods are public — newer tests in this package use package-private (see OptimisticLockRetryPolicyTest, AggregateCompletionOnlyTwoTest) |
| 4 | Should fix | Unused variable callerThread in testSyncRetryHappensInSameThread |

See inline comments for details and suggested fixes.

This review covers project conventions and test quality. It does not replace specialized tools (CodeRabbit, SonarCloud, etc.).

Resubmitting with corrected inline comments

gnodet

Second Review: CAMEL-23352 — `optimisticLockingSyncRetry` option for Aggregate EIP

Claude Code on behalf of Guillaume Nodet

Thank you for addressing the first round of feedback — the rename to optimisticLockingSyncRetry, the improved Javadoc, and the two additional tests (nonzero delay + interrupt path) are all solid improvements. The core implementation in AggregateProcessor.doInOptimisticLock() looks correct: the synchronous doDelay() + continue path cleanly preserves the calling thread, and the InterruptedException handling is done properly with Thread.currentThread().interrupt().

This second round focuses on the test class, specifically around style conventions and test robustness.

Summary of Findings

#	Severity	Finding
1	Must fix	`Thread.sleep(100)` in `testSyncRetryInterrupted` — use Awaitility (project rule, also flagged by @Croway)
2	Must fix	Conditional assertions silently pass — `if (aggregateThreadName != null)` and `if (result != null && ...)` mean the test can succeed without ever verifying the core behavior
3	Should fix	Test class and methods are `public` — newer tests in this package use package-private (see `OptimisticLockRetryPolicyTest`, `AggregateCompletionOnlyTwoTest`)
4	Should fix	Unused variable `callerThread` in `testSyncRetryHappensInSameThread`

See inline comments for details and suggested fixes.

This review covers project conventions and test quality. It does not replace specialized tools (CodeRabbit, SonarCloud, etc.).

Resubmitting with correct line positioning

gnodet

Second Review: CAMEL-23352 — `optimisticLockingSyncRetry` option for Aggregate EIP

Claude Code on behalf of Guillaume Nodet

Thank you for addressing the first round of feedback — the rename to optimisticLockingSyncRetry, the improved Javadoc, and the two additional tests (nonzero delay + interrupt path) are all solid improvements. The core implementation in AggregateProcessor.doInOptimisticLock() looks correct: the synchronous doDelay() + continue path cleanly preserves the calling thread, and the InterruptedException handling is done properly with Thread.currentThread().interrupt().

This second round focuses on the test class, specifically around style conventions and test robustness.

Summary of Findings

#	Severity	Finding
1	Must fix	`Thread.sleep(100)` in `testSyncRetryInterrupted` — use Awaitility (project rule, also flagged by @Croway)
2	Must fix	Conditional assertions silently pass — `if (aggregateThreadName != null)` and `if (result != null && ...)` mean the test can succeed without ever verifying the core behavior
3	Should fix	Test class and methods are `public` — newer tests in this package use package-private (see `OptimisticLockRetryPolicyTest`, `AggregateCompletionOnlyTwoTest`)
4	Should fix	Unused variable `callerThread` in `testSyncRetryHappensInSameThread`

See inline comments for details and suggested fixes.

This review covers project conventions and test quality. It does not replace specialized tools (CodeRabbit, SonarCloud, etc.).

code-massel · 2026-05-07T13:30:02Z

Thanks for the second round of feedback — all items addressed and pushed (10bd777).

Test class fixes:

Replaced Thread.sleep(100) in testSyncRetryInterrupted with Awaitility.await().atMost(5s).until(() -> interruptCounter.get() > 0) — no more timing-dependent sleep before interrupt.
Replaced conditional if (aggregateThreadName != null) and if (result != null && ...) assertions with assertNotNull(...) + unconditional assertions — tests now actually fail if the core behavior is not exercised.
Removed public from class and all test methods — now package-private, consistent with AggregateCompletionOnlyTwoTest and others in the same package.
Removed unused callerThread variable in testSyncRetryHappensInSameThread.

Missing generated file:

camelYamlDsl-canonical.json was not present on the branch because the file was added to main after the branch was created. Rebased the branch onto current main and regenerated — the file now includes the optimisticLockingSyncRetry option (6 insertions).

All 4 tests pass locally. Let me know if there is anything else to address.

davsclaus · 2026-05-07T14:10:12Z

Sorry but its safer to do a full rebuild when you change EIPs or whatnot as it affect other DSLs, catalog etc

There are uncommitted changes
HEAD detached at pull/22769/merge
Changes not staged for commit:
(use "git add ..." to update what will be committed)
(use "git restore ..." to discard changes in working directory)
modified: catalog/camel-catalog/src/generated/resources/org/apache/camel/catalog/schemas/camel-spring.xsd
modified: catalog/camel-catalog/src/generated/resources/org/apache/camel/catalog/schemas/camel-xml-io.xsd

When optimisticLocking is enabled, retries are normally scheduled asynchronously on a background ScheduledExecutorService, which switches threads and breaks transactional use cases where the aggregation repository and inbound message store must commit/rollback within a single transaction boundary. The new syncOptimisticRetry flag (default false) causes the retry delay to execute via Thread.sleep on the calling thread instead, preserving the transaction context. This restores the Camel 2.x behavior for users who need it, without changing the default async behavior.

Rename syncOptimisticRetry → optimisticLockingSyncRetry for consistency with the existing optimisticLocking option naming. Update Javadoc to note the flag only takes effect when optimisticLocking() is enabled. Add tests for non-zero retry delay and thread interruption scenarios. Regenerate catalog metadata.

…chemas)

- Remove public modifiers from class and all test methods (package-private convention) - Remove unused callerThread variable in testSyncRetryHappensInSameThread - Replace conditional if-assertions with assertNotNull + direct assertions - Replace Thread.sleep(100) with Awaitility.await() in testSyncRetryInterrupted

…ncRetry File was added to main after branch was created. Regenerated after rebasing onto main; includes the new optimisticLockingSyncRetry option.

Restore 16 files where the build corrupted Unicode characters (en-dash, smart quotes, ❌, ✅) to replacement sequences due to missing UTF-8 encoding during generation.

github-actions Bot added the core label Apr 25, 2026

davsclaus force-pushed the CAMEL-23352 branch from edf96bb to c6d2b01 Compare April 25, 2026 10:52

gnodet requested changes Apr 27, 2026

View reviewed changes

code-massel requested review from davsclaus and gnodet May 5, 2026 20:19

github-actions Bot added catalog dsl labels May 5, 2026

davsclaus approved these changes May 6, 2026

View reviewed changes

Croway reviewed May 6, 2026

View reviewed changes

gnodet previously requested changes May 7, 2026

View reviewed changes

gnodet requested changes May 7, 2026

View reviewed changes

gnodet reviewed May 7, 2026

View reviewed changes

Comment thread ...rc/test/java/org/apache/camel/processor/aggregator/AggregateOptimisticLockSyncRetryTest.java Outdated

gnodet reviewed May 7, 2026

View reviewed changes

Comment thread ...rc/test/java/org/apache/camel/processor/aggregator/AggregateOptimisticLockSyncRetryTest.java Outdated

code-massel force-pushed the CAMEL-23352 branch from 093ba03 to 10bd777 Compare May 7, 2026 13:13

marcelatsap added 5 commits May 8, 2026 07:53

CAMEL-23352: Regenerate downstream artifacts (YAML DSL, XML IO, XSD s…

da675ee

…chemas)

CAMEL-23352: Add camelYamlDsl-canonical.json with optimisticLockingSy…

69519c1

…ncRetry File was added to main after branch was created. Regenerated after rebasing onto main; includes the new optimisticLockingSyncRetry option.

marcelatsap added 2 commits May 8, 2026 07:53

CAMEL-23352: Regenerate all artifacts after rebase onto main

711f3b0

CAMEL-23352: Fix encoding corruption in regenerated artifacts

74cb64b

Restore 16 files where the build corrupted Unicode characters (en-dash, smart quotes, ❌, ✅) to replacement sequences due to missing UTF-8 encoding during generation.

davsclaus force-pushed the CAMEL-23352 branch from a005dd0 to 74cb64b Compare May 8, 2026 05:53

davsclaus approved these changes May 8, 2026

View reviewed changes

davsclaus merged commit b357f28 into apache:main May 8, 2026
3 of 6 checks passed

Conversation

code-massel commented Apr 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Target

Tracking

Apache Camel coding standards and style

Uh oh!

github-actions Bot commented Apr 25, 2026

Uh oh!

davsclaus commented Apr 25, 2026

Uh oh!

davsclaus commented Apr 25, 2026

Uh oh!

gnodet left a comment

Choose a reason for hiding this comment

Review: CAMEL-23352 — Add syncOptimisticRetry option to Aggregate EIP

Blocker: Missing generated files

Implementation

Uh oh!

Uh oh!

Uh oh!

Uh oh!

code-massel commented May 5, 2026

Uh oh!

Croway May 6, 2026

Choose a reason for hiding this comment

Uh oh!

Croway commented May 6, 2026

Uh oh!

davsclaus commented May 7, 2026

Uh oh!

gnodet left a comment

Choose a reason for hiding this comment

Second Review: CAMEL-23352 — optimisticLockingSyncRetry option for Aggregate EIP

Summary of Findings

Uh oh!

gnodet left a comment

Choose a reason for hiding this comment

Second Review: CAMEL-23352 — optimisticLockingSyncRetry option for Aggregate EIP

Summary of Findings

Uh oh!

gnodet left a comment

Choose a reason for hiding this comment

Second Review: CAMEL-23352 — optimisticLockingSyncRetry option for Aggregate EIP

Summary of Findings

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

code-massel commented May 7, 2026

Uh oh!

davsclaus commented May 7, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

code-massel commented Apr 24, 2026 •

edited

Loading

Review: CAMEL-23352 — Add `syncOptimisticRetry` option to Aggregate EIP

Second Review: CAMEL-23352 — `optimisticLockingSyncRetry` option for Aggregate EIP

Second Review: CAMEL-23352 — `optimisticLockingSyncRetry` option for Aggregate EIP

Second Review: CAMEL-23352 — `optimisticLockingSyncRetry` option for Aggregate EIP