oscerd commented on code in PR #27834:
URL: https://github.com/apache/camel/pull/27834#discussion_r4280267391
##########
components/camel-azure/camel-azure-servicebus/src/main/java/org/apache/camel/component/azure/servicebus/ServiceBusConsumer.java:
##########
@@ -46,13 +46,22 @@
import org.apache.camel.support.PluginHelper;
import org.apache.camel.support.SynchronizationAdapter;
import org.apache.camel.util.ObjectHelper;
+import org.apache.camel.util.StringHelper;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
public class ServiceBusConsumer extends DefaultConsumer implements
ShutdownAware {
private static final Logger LOG =
LoggerFactory.getLogger(ServiceBusConsumer.class);
private static final int LOCK_RENEW_INTERVAL_SECONDS = 10;
+ // Azure recommends putting the exception type in the dead-letter reason
and its stack trace in the description,
+ // but warns that doing so "might result in some messages exceeding the
256 KB quota limit for the Standard tier".
+ // A rejected dead-letter request leaves the message locked, so cap both
rather than risk the quota: the head of a
+ // stack trace carries the cause, the tail is error-handler frames.
+ private static final int MAX_DEAD_LETTER_REASON_LENGTH = 1024;
+ private static final int MAX_DEAD_LETTER_DESCRIPTION_LENGTH = 8192;
Review Comment:
You are right and I was wrong — thanks for pushing back with the source
rather than the doc. Fixed in `7941ee6`: both fields are now capped at 4096.
I verified it rather than take it second-hand, and it holds:
- `azure-sdk-for-net`,
`sdk/servicebus/Azure.Messaging.ServiceBus/src/Constants.cs` — `public const
int MaxDeadLetterReasonLength = 4096;`
- the same repo, `Amqp/AmqpReceiver.cs`, as the first statements of
`DeadLetterInternalAsync` — `Argument.AssertNotTooLong(deadLetterReason,
Constants.MaxDeadLetterReasonLength, ...)` and
`Argument.AssertNotTooLong(deadLetterErrorDescription,
Constants.MaxDeadLetterReasonLength, ...)`. Both fields, against the reason
constant, before either disposition path.
- `azure-sdk-for-js`, `sdk/core/core-amqp/src/util/constants.ts` —
`maxDeadLetterReasonLength: 4096`.
**Why I got it wrong**, since it is the interesting part. I originally cited
4096 from the .NET `DeadLetterMessageAsync` reference page. Then I went looking
for corroboration, found none on the dead-letter concept page or the quotas
page, found that the Java SDK's `DeadLetterOptions` does no validation, and
retracted the number on CAMEL-25660. That inference rested on two absences — no
figure in the prose docs, no check in the Java SDK — and read absence of
evidence as evidence of absence. The Java SDK turned out to be the outlier
among three. I have withdrawn the retraction on the issue with the quotes above.
The 256 KB message quota is still real and still a reason to bound a stack
trace; it is just not the tightest constraint. The class comment now gives
both, with 4096 as the operative one.
One difference from your suggestion worth flagging: rather than setting the
constant to 4093 so that the suffix brings it to 4096, I kept
`MAX_DEAD_LETTER_TEXT_LENGTH = 4096` — the number a reader can match against
the SDK constant — and subtract the suffix where it is applied:
```java
private String limitToDeadLetterLength(String value) {
return StringHelper.limitLength(value, MAX_DEAD_LETTER_TEXT_LENGTH -
TRUNCATED_SUFFIX.length(), TRUNCATED_SUFFIX);
}
```
Same output, and `4093` never appears as a magic number needing its own
explanation. The test asserts `hasSizeLessThanOrEqualTo(4096)` on both fields,
and reverting the constant to 8192 fails it on all 3 runs.
_Claude Code on behalf of oscerd_
##########
components/camel-azure/camel-azure-servicebus/src/main/java/org/apache/camel/component/azure/servicebus/ServiceBusConsumer.java:
##########
@@ -405,5 +407,43 @@ public void onFailure(Exchange exchange) {
pendingExchanges.decrementAndGet();
}
}
+
+ /**
+ * Describes the failure for the dead-letter queue, bounded so that
the diagnostics cannot be what makes the
+ * broker reject the message.
+ */
+ private DeadLetterOptions deadLetterOptions(Exception cause) {
+ String stackTrace = Arrays.stream(cause.getStackTrace())
+ .map(StackTraceElement::toString)
+ .collect(Collectors.joining("\n"));
+
+ return new DeadLetterOptions()
+ .setDeadLetterReason(StringHelper.limitLength(
+ String.format("%s: %s",
cause.getClass().getName(), cause.getMessage()),
+ MAX_DEAD_LETTER_REASON_LENGTH, TRUNCATED_SUFFIX))
+ .setDeadLetterErrorDescription(StringHelper.limitLength(
+ stackTrace, MAX_DEAD_LETTER_DESCRIPTION_LENGTH,
TRUNCATED_SUFFIX));
Review Comment:
Correct, and it made the cap wrong by three characters.
`StringHelper.limitLength(s, maxLength, suffix)` cuts to `maxLength` and *then*
appends, so it returns `maxLength + suffix.length()`.
Fixed in `7941ee6` by subtracting the suffix where the cap is applied, so
the constant can stay the number that matches the SDK:
```java
return StringHelper.limitLength(value, MAX_DEAD_LETTER_TEXT_LENGTH -
TRUNCATED_SUFFIX.length(), TRUNCATED_SUFFIX);
```
The test now asserts `hasSizeLessThanOrEqualTo(4096)` rather than the `max +
3` it had before — which, embarrassingly, encoded the bug as the expectation.
_Claude Code on behalf of oscerd_
##########
components/camel-azure/camel-azure-servicebus/src/main/java/org/apache/camel/component/azure/servicebus/ServiceBusConsumer.java:
##########
@@ -405,5 +407,43 @@ public void onFailure(Exchange exchange) {
pendingExchanges.decrementAndGet();
}
}
+
+ /**
+ * Describes the failure for the dead-letter queue, bounded so that
the diagnostics cannot be what makes the
+ * broker reject the message.
+ */
+ private DeadLetterOptions deadLetterOptions(Exception cause) {
+ String stackTrace = Arrays.stream(cause.getStackTrace())
+ .map(StackTraceElement::toString)
+ .collect(Collectors.joining("\n"));
+
+ return new DeadLetterOptions()
+ .setDeadLetterReason(StringHelper.limitLength(
+ String.format("%s: %s",
cause.getClass().getName(), cause.getMessage()),
+ MAX_DEAD_LETTER_REASON_LENGTH, TRUNCATED_SUFFIX))
+ .setDeadLetterErrorDescription(StringHelper.limitLength(
+ stackTrace, MAX_DEAD_LETTER_DESCRIPTION_LENGTH,
TRUNCATED_SUFFIX));
+ }
+
+ /**
+ * Dead-letters the message, reporting a rejected request instead of
letting it leave this synchronization.
+ * <p/>
+ * Nothing else would report it: the exception would surface inside
the Azure SDK's processor callback, the
+ * message would stay locked until the lock expired, and Service Bus
would redeliver it until
+ * {@code maxDeliveryCount} was reached - dead-lettering it with its
own reason, which is the opposite of what
+ * {@code enableDeadLettering} is for.
Review Comment:
Confirmed and corrected in `7941ee6`. `UnitOfWorkHelper.doneSynchronization`
wraps the `synchronization.onFailure(exchange)` call and catches:
```java
} catch (Exception e) {
// must catch exceptions to ensure all synchronizations have a chance to
run
LOG.warn("Exception occurred during onCompletion. This exception will be
ignored.", e);
}
```
So nothing reaches the SDK callback, and my javadoc, the PR description and
the Jira all overstated it. The javadoc now says what the change actually buys:
the failure is reported through the consumer's `ExceptionHandler`, where it is
attributable to this endpoint and routable, instead of a generic core warning
that names `onCompletion`. The part that does not change either way — the
message is unsettled, stays locked, is redelivered, and ends up dead-lettered
as `MaxDeliveryCountExceeded` — is still stated, since that is the
operator-visible consequence.
_Claude Code on behalf of oscerd_
##########
components/camel-azure/camel-azure-servicebus/src/main/java/org/apache/camel/component/azure/servicebus/ServiceBusConsumer.java:
##########
@@ -385,17 +394,10 @@ public void onFailure(Exchange exchange) {
if (getConfiguration().getServiceBusReceiveMode() ==
ServiceBusReceiveMode.PEEK_LOCK) {
if (getConfiguration().isEnableDeadLettering() &&
(ObjectHelper.isEmpty(getConfiguration().getSubQueue())
||
ObjectHelper.equal(getConfiguration().getSubQueue(), SubQueue.NONE))) {
- DeadLetterOptions deadLetterOptions = new
DeadLetterOptions();
if (cause != null) {
- deadLetterOptions
- .setDeadLetterReason(
- String.format("%s: %s",
cause.getClass().getName(), cause.getMessage()));
-
deadLetterOptions.setDeadLetterErrorDescription(Arrays.stream(cause.getStackTrace())
- .map(StackTraceElement::toString)
- .collect(Collectors.joining("\n")));
- messageContext.deadLetter(deadLetterOptions);
+ deadLetter(exchange, deadLetterOptions(cause));
} else {
- messageContext.deadLetter();
+ deadLetter(exchange, null);
}
} else {
messageContext.abandon();
Review Comment:
Taken, in `7941ee6`. Both settlement calls in `onFailure` now go through one
helper:
```java
settle(exchange, "dead-letter", () -> messageContext.deadLetter(options));
...
settle(exchange, "abandon", messageContext::abandon);
```
New test `synchronizationReportsARejectedAbandonInsteadOfThrowing` covers
it: dead-lettering off so the failure path abandons, `abandon()` stubbed to
throw, asserting the exception handler receives it.
For completeness, the other `abandon()` in this class — in the catch around
`getAsyncProcessor().process(...)` — is already wrapped in its own try/catch,
so it was the only unguarded one.
_Claude Code on behalf of oscerd_
##########
docs/user-manual/modules/ROOT/pages/camel-4x-upgrade-guide-4_24.adoc:
##########
@@ -414,6 +414,20 @@ Previously it returned only the first chunk of the event
log (at most 500 events
any sign. A wide `after`/`before` window now returns every event in one list:
that is one request per 500 events plus
one that finds the end of the log, and more memory. Narrow the window when the
event log is large.
+=== camel-azure-servicebus - the dead-letter reason and description are bounded
+
+When the consumer dead-letters a message after a failed route with
`enableDeadLettering=true`, the reason and the
+error description it writes are now capped - the reason at 1024 characters and
the description, which holds the stack
+trace, at 8192. A value that was cut ends with `...` so it is clear it was
truncated rather than that the trace ended
+there.
+
+Azure recommends putting the exception type in the reason and its stack trace
in the description, but warns that doing
+so can push a message past the 256 KB quota of the Standard tier. Nothing
bounded either field before, so a long
+stack trace could make the broker reject the dead-letter request - and because
that rejection was not handled, the
+message was then left locked and redelivered until `maxDeliveryCount` was
reached, at which point the broker
+dead-lettered it with its own `MaxDeliveryCountExceeded` reason. A rejected
request is now reported through the
+consumer's exception handler instead.
Review Comment:
Agreed, and the old wording was inaccurate as well as long — it said the
rejection "was not handled", when the unit of work was logging a WARN for it
all along. Cut to:
> When the consumer dead-letters a message after a failed route with
`enableDeadLettering=true`, the reason and the error description it writes are
now capped at 4096 characters, the limit the Azure SDKs apply to both fields. A
value that was cut ends with `...`. The description holds the stack trace, so a
deep one is now truncated rather than sent in full and rejected by the broker.
>
> A settlement request the broker rejects is now reported through the
consumer's exception handler.
Kept the first paragraph because the truncation is the part an upgrading
user can actually observe — a stack trace in their dead-letter queue that now
stops at 4096 characters.
_Claude Code on behalf of oscerd_
--
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]
