oscerd commented on code in PR #27834:
URL: https://github.com/apache/camel/pull/27834#discussion_r4280268897
##########
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:
Right, and confirmed at the source — fixed in `7941ee6`. Both fields are now
capped at 4096, the value the .NET and JavaScript SDKs enforce
(`Constants.MaxDeadLetterReasonLength`, asserted on the description as well in
`AmqpReceiver.DeadLetterInternalAsync`). Details and the quotes are on
@davsclaus's thread about the same line, including why I had wrongly retracted
that figure earlier.
_Claude Code on behalf of oscerd_
##########
components/camel-azure/camel-azure-servicebus/src/test/java/org/apache/camel/component/azure/servicebus/ServiceBusConsumerTest.java:
##########
@@ -385,6 +385,78 @@ private void
synchronizationDeadLettersMessageWithOptionsWhenExceptionPresent(Su
}
}
+ @Test
+ void synchronizationBoundsTheDeadLetterReasonAndDescription() throws
Exception {
+ try (ServiceBusConsumer consumer = new ServiceBusConsumer(endpoint,
processor)) {
+
when(configuration.getServiceBusReceiveMode()).thenReturn(ServiceBusReceiveMode.PEEK_LOCK);
+ when(configuration.isEnableDeadLettering()).thenReturn(true);
+
+ consumer.doStart();
+ when(messageContext.getMessage()).thenReturn(message);
+ processMessageCaptor.getValue().accept(messageContext);
+
+ Exchange exchange = exchangeCaptor.getValue();
+
exchange.setException(exceptionWithALongMessageAndADeepStackTrace());
+
+ Synchronization synchronization =
exchange.getExchangeExtension().handoverCompletions().get(0);
+ synchronization.onFailure(exchange);
+
+ ArgumentCaptor<DeadLetterOptions> captor = ArgumentCaptor.captor();
+ verify(messageContext).deadLetter(captor.capture());
+ DeadLetterOptions options = captor.getValue();
+
+ // Azure asks for the exception type and its stack trace here, but
warns the result can push the message
+ // past the 256 KB quota of the Standard tier - so both are
capped, and visibly so
+ assertThat(options.getDeadLetterReason())
+ .hasSizeLessThanOrEqualTo(1024 + 3)
+ .startsWith(IllegalStateException.class.getName())
+ .endsWith("...");
+ assertThat(options.getDeadLetterErrorDescription())
+ .hasSizeLessThanOrEqualTo(8192 + 3)
Review Comment:
The assertion is now `hasSizeLessThanOrEqualTo(4096)` on both fields, in
`7941ee6`.
I took a slightly different route to the same place: rather than setting the
constant to `4093` so the suffix lands it on 4096, the constant stays
`MAX_DEAD_LETTER_TEXT_LENGTH = 4096` — matching the SDK constant a reader would
compare it against — and the suffix is subtracted where it is applied,
`limitLength(value, MAX - TRUNCATED_SUFFIX.length(), TRUNCATED_SUFFIX)`. That
keeps `4093` from appearing as a magic number that would itself need a comment.
The exception used by the test now carries an 8192-character message and 512
frames, so both fields are comfortably over the cap before truncation.
_Claude Code on behalf of oscerd_
##########
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".
Review Comment:
Fair — the comment explained the quota I had found and not the limit that
actually binds. Rewritten in `7941ee6` to lead with the tighter one:
```java
// Azure recommends putting the exception type in the dead-letter reason and
its stack trace in the description,
// but both are length-limited: the .NET and JavaScript SDKs cap them at
4096 (MaxDeadLetterReasonLength, applied
// to the description as well), and Azure separately warns that a stack
trace here can push a message past the
// 256 KB quota of the Standard tier. The Java SDK does not check, so an
over-long value only fails at the broker -
// which leaves the message locked. Cap it here instead: the head of a stack
trace carries the cause, the tail is
// error-handler frames.
```
Both constraints are worth keeping: 4096 is what bounds these two fields,
the 256 KB quota is why putting a stack trace in a message is worth thinking
about at all.
_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]
