allthingssecurity commented on PR #28646:
URL: https://github.com/apache/camel/pull/28646#issuecomment-6491048835
@davsclaus thanks for the review. Main is merged in 26ecb9a5a2ee (a merge
commit, no force-push) and the changes are in 1696c5009cd0.
1. **JTA.** You are right that the text was wrong, but the hook is not
skipping the JTA error handling. camel-jta's `TransactionErrorHandler` is not
the error handler of a step: it is the output of `JtaTransactionErrorHandler`
(a `RedeliveryErrorHandler`), which is the processor of the `transacted` step.
`JtaTransactionPolicy.beforeWrap` sets `inheritErrorHandler=false`, so the
`transacted` step and its direct children have no error handler, and their
channels ask the hook. A failure of a direct child, from the hook or from the
processor, rolls the transaction back and goes to `onException` and the
redelivery of the whole transaction. A step nested in an EIP inside the
transaction (`filter`, `split`, ...) gets the route's error handler, so it is
redelivered inside the transaction. A failure from the hook at the `transacted`
step itself happens before the transaction begins and is not handled. The
javadoc, `chaos.adoc` and the PR body now say this. The new `StepHookJtaTest`
in came
l-jta covers the three cases, each with a hook failure next to a processor
failure (same result). No code change in camel-jta was needed.
2. **Merge.** Merged origin/main and rebuilt. The generated
`ExtendedCamelContextConfigurer` has both `sendHook` and `stepHook`, as the
build writes it.
3. **Hot path.** The error handler, the channel and the JMX-side processor
now read the hook from their own `camelContext` field, and `StepHelper.process`
takes the hook as a parameter. With no hook, the per-step branch left is a null
check on a plain field. Then, only where the hook is asked, one volatile read.
JMH 1.37 (camel-performance-tests is not available offline here, so I ran
a small JMH benchmark on the `target/classes` of main and of the branch): 10
no-op `process` steps through the direct producer, ns per exchange, 3 forks,
main / branch:
- default error handler: 844 ± 19 / 885 ± 18 without JMX, 1468 ± 38 /
1483 ± 30 with JMX
- `maximumRedeliveries(1)`: 951 ± 42 / 958 ± 26 without JMX, 1553 ± 26 /
1553 ± 24 with JMX
- `noErrorHandler()`: 556 ± 8 / 558 ± 14 without JMX, 1197 ± 27 / 1173 ±
11 with JMX
So the cost is not zero. With the default error handler and no JMX it is
about 2 to 4 ns per step (an earlier run gave 892 / 910). Elsewhere it is below
the run-to-run spread; the machine was not idle.
To make it zero: decide where the hook is asked only when a hook is
already installed at route start, and wrap the error handler's output through
the existing `changeOutput`. With no hook there is then no field, no branch and
no extra processor, also with JMX. The limit: a hook installed later is not
asked by routes already running. camel-chaos installs its hook at startup, so
that limit is fine for it. Shall I switch to that?
4. **API.** `useStepHook` is a default method on an interface whose javadoc
says Camel uses it internally during route initialization. The channel already
uses it the same way for JMX (`changeOutput`). An error handler that does not
override it keeps today's behaviour. The cleaner alternative is the option in
point 3: no new SPI method, only `changeOutput`. It only works with that
start-time limit, though; without the limit it adds a processor to every step.
`nodeId` is now `@Nullable`. The javadoc now says the channel calls it again
with `null` when JMX takes over, and `beforeStep` lists its return values:
`0`/`PROCEED`, a positive delay, and negatives reserved
(`IllegalArgumentException`).
5. **intercept().** The javadoc says the hook is asked after the channel's
advices and before the `InterceptStrategy` wrappers, so a failed step also
skips its `intercept()` processors. A new `StepHookTest` test covers it for
both places that ask. `chaos.adoc` "Order of work" and the "Every step" row are
updated.
Tests on 1696c5009cd0: camel-core `org.apache.camel.processor` (incl. error
handlers) and `*ErrorHandler*` 2860, camel-management 461, camel-jta 15, 0
failures; `StepHookTest` and `StepHookJtaTest` 20x clean. Negative controls:
mutations of the channel fail the new tests.
_Claude Code on behalf of allthingssecurity_
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
