gemmellr commented on a change in pull request #2847: ARTEMIS-2494: [AMQP]
Allow Modified disposition to be used signal address full to a sending peer
URL: https://github.com/apache/activemq-artemis/pull/2847#discussion_r327656358
##########
File path:
artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/proton/ProtonServerReceiverContext.java
##########
@@ -326,6 +325,37 @@ private void actualDelivery(Delivery delivery, Receiver
receiver, ReadableBuffer
}
}
+ private DeliveryState determineDeliveryState(final Source source, final
boolean useModified, final Exception e) {
+ Outcome defaultOutcome = getEffectiveDefaultOutcome(source);
+ if (e instanceof ActiveMQAddressFullException && useModified &&
+ (outcomeSupported(source, Modified.DESCRIPTOR_SYMBOL) ||
defaultOutcome instanceof Modified)) {
+ Modified modified = new Modified();
+ modified.setDeliveryFailed(true);
+ modified.setUndeliverableHere(false);
+ return modified;
+ } else {
+ if (outcomeSupported(source, Rejected.DESCRIPTOR_SYMBOL) ||
defaultOutcome instanceof Rejected) {
+ ErrorCondition condition = new ErrorCondition();
+
+ // Set condition
+ if (e instanceof ActiveMQSecurityException) {
+ condition.setCondition(AmqpError.UNAUTHORIZED_ACCESS);
+ } else {
+ condition.setCondition(Symbol.valueOf("failed"));
+ }
+ condition.setDescription(e.getMessage());
+
+ Rejected rejected = new Rejected();
+ rejected.setError(condition);
+ return rejected;
+ } else if (source.getDefaultOutcome() instanceof DeliveryState) {
+ return ((DeliveryState) source.getDefaultOutcome());
+ } else {
+ return Accepted.getInstance();
Review comment:
I think introducing this last bit as a new default behaviour will turn out
to be very painful, and so think it should be gated to opt-in instead. It also
feels like it somewhat goes against the fact this change introduces a toggle to
mostly avoid changing the existing default questionable behaviour.
I've come to realise since seeing this PR that various other clients/servers
are not setting the outcomes at all, though both using and expecting the full
range of outcomes, and therefore it seems this change will have the effect of
changing the default behaviour from the broker rejecting, to the broker
accepting the message and probably dropping it on the floor.
I realise this is actually implementing the [strange] behaviour the AMQP 1.0
spec outlines for the outcomes / default-outcome field handling, but after all
this time I think it would be a bad idea to introduce it by default knowing
many existing peers will fall foul of it.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
