CAMEL-23352: Add syncOptimisticRetry option to Aggregate EIP#22769
CAMEL-23352: Add syncOptimisticRetry option to Aggregate EIP#22769code-massel wants to merge 1 commit intoapache:mainfrom
Conversation
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
|
when you add new options to EIPs you generally need to rebuild the entire code base |
|
for example run from root |
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.
gnodet
left a comment
There was a problem hiding this comment.
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 -DskipTestsThen check git status and commit all generated file changes. This will update at minimum:
core/camel-core-model/src/generated/— JSON model metadata, configurersdsl/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
syncOptimisticRetrybranch correctly callsdoDelay(attempt)thencontinues the retry loop on the calling thread. InterruptedExceptionhandling correctly restores the interrupt flag and propagates viaexchange.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
| private String optimisticLocking; | ||
| @XmlAttribute | ||
| @Metadata(label = "advanced", javaType = "java.lang.Boolean", defaultValue = "false") | ||
| private String syncOptimisticRetry; |
There was a problem hiding this comment.
Nit / naming: consider optimisticLockingSyncRetry (or optimisticLockRetrySync) to group it visually with the existing optimisticLocking field in docs and configuration. syncOptimisticRetry reads a bit ambiguously ("synchronize" vs "synchronous"). Not a blocker — just a readability suggestion.
There was a problem hiding this comment.
I like the suggested rename of the option
| /** | ||
| * When optimistic locking is enabled, retries happen synchronously in the same thread instead of being scheduled on | ||
| * a background thread. This preserves transaction context for repositories that require single-thread transactional | ||
| * guarantees. | ||
| */ |
There was a problem hiding this comment.
Nice Javadoc. Consider adding a note that this option only takes effect when optimisticLocking is also enabled:
| /** | |
| * When optimistic locking is enabled, retries happen synchronously in the same thread instead of being scheduled on | |
| * a background thread. This preserves transaction context for repositories that require single-thread transactional | |
| * guarantees. | |
| */ | |
| /** | |
| * When optimistic locking is enabled, retries happen synchronously in the same thread instead of being scheduled on | |
| * a background thread. This preserves transaction context for repositories that require single-thread transactional | |
| * guarantees. Only takes effect when {@link #optimisticLocking()} is also enabled. | |
| */ |
| .optimisticLocking() | ||
| .syncOptimisticRetry() | ||
| .optimisticLockRetryPolicy( | ||
| new OptimisticLockRetryPolicy().maximumRetries(10).retryDelay(0)) |
There was a problem hiding this comment.
Good test coverage overall. One observation: retryDelay(0) means doDelay() returns immediately without sleeping, so the Thread.sleep + InterruptedException code path isn't exercised.
Consider adding a small test with a nonzero delay (e.g., retryDelay(10)) to verify the synchronous sleep actually happens on the calling thread under realistic conditions. A test that interrupts the thread during retry would further strengthen coverage of the InterruptedException branch. Not a blocker, but would increase confidence.
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
optimisticLockingis enabled on the Aggregate EIP, retry failures are scheduled asynchronously on a backgroundScheduledExecutorService. 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
syncOptimisticRetryoption (defaultfalse) to theaggregateEIP. When enabled, the retry delay runs viaThread.sleepon the calling thread instead of scheduling on a background executor, preserving the transaction context. Default isfalseso existing routes are unaffected.Target
mainbranch)Tracking
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 -DskipTestslocally from root folder and I have committed all auto-generated changes.