gnodet-bot commented on code in PR #27834:
URL: https://github.com/apache/camel/pull/27834#discussion_r4280243948
##########
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:
⚠️ **Bug — description limit exceeds the Azure broker cap.**
`MAX_DEAD_LETTER_DESCRIPTION_LENGTH = 8192` will make the broker reject any
dead-letter request where the description overflows 4 096 characters. Both the
.NET SDK (`Constants.MaxDeadLetterReasonLength = 4096`, used for _both_ fields
in `AmqpReceiver.cs`) and the JS SDK (`maxDeadLetterReasonLength: 4096` in
`@azure/core-amqp`) document this hard limit.
Result with the current value: the capped description is at most 8 195
characters (8 192 + `"..."`), which is double the broker limit — so on a long
stack trace the dead-letter request still gets rejected, and the message stays
locked.
```suggestion
private static final int MAX_DEAD_LETTER_DESCRIPTION_LENGTH = 4093;
```
`4093 + 3 ("...") = 4096` — fits the broker limit exactly.
##########
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:
Once `MAX_DEAD_LETTER_DESCRIPTION_LENGTH` is corrected to `4093`, the bound
on this assertion should follow:
```suggestion
.hasSizeLessThanOrEqualTo(4093 + 3)
```
Also consider asserting the upper bound is ≤ 4096 directly (the broker
limit) rather than `constant + suffix_len`, so the test documents the
constraint rather than the implementation detail.
##########
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:
💡 The class-level comment explains the 256 KB _total-message_ size concern,
but the tighter per-field 4 096-character broker limit (which drives the cap
values) isn't mentioned. Adding a reference to that limit would make the
constant values self-explanatory:
```suggestion
// 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".
// Azure Service Bus also enforces a hard 4 096-character limit on both
dead-letter fields (documented in the
// .NET SDK as Constants.MaxDeadLetterReasonLength). A rejected
dead-letter request leaves the message locked,
// so cap both to stay within the broker limit: the head of a stack
trace carries the cause, the tail is
// error-handler frames.
```
--
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]
