davsclaus commented on code in PR #27834:
URL: https://github.com/apache/camel/pull/27834#discussion_r4275137236
##########
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:
`limitLength` appends the suffix after cutting to `maxLength`, so the result
can be `MAX + 3` characters long. If the cap is meant as a hard limit (for
example 4096), use `MAX_... - TRUNCATED_SUFFIX.length()` as the `maxLength`.
##########
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:
Non-blocking: `abandon()` has the same unguarded shape, in the same method.
A failure here only shows up as the generic WARN from the unit of work. You
could route it through `getExceptionHandler()` the same way as the dead-letter
call.
##########
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:
Small correction: an exception from `onFailure` does not reach the Azure SDK
callback. `UnitOfWorkHelper.doneSynchronization` catches it and logs `WARN
Exception occurred during onCompletion. This exception will be ignored.` The
new value here is that it goes to the consumer's `ExceptionHandler`
(bridgeErrorHandler, custom handlers) with a clearer message. Could the javadoc
say that instead?
##########
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:
The upgrade guide is for migration notes, so this paragraph could be a
single sentence: "A dead-letter request the broker rejects is now reported
through the consumer's exception handler." Also, "that rejection was not
handled" is not quite right: Camel's unit of work caught it and logged a WARN.
##########
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:
The .NET SDK checks both the reason and the description against
`Constants.MaxDeadLetterReasonLength = 4096` (`AmqpReceiver.cs`,
`Argument.AssertNotTooLong`), and the JS SDK has the same 4096 constant. With
8192, a typical Camel stack trace of 4-8K characters would still go above that
limit. Suggest 4096 for the description (and 4096 or lower for the reason).
--
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]
