Github user gemmellr commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/690#discussion_r73343218
--- Diff:
artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/proton/plug/ProtonSessionIntegrationCallback.java
---
@@ -351,18 +353,35 @@ public void serverSend(final Receiver receiver,
recoverContext();
PagingStore store =
manager.getServer().getPagingManager().getPageStore(message.getAddress());
- if (store.isFull() && store.getAddressFullMessagePolicy() ==
AddressFullMessagePolicy.BLOCK) {
- ErrorCondition ec = new
ErrorCondition(AmqpError.RESOURCE_LIMIT_EXCEEDED, "Address is full: " +
message.getAddress());
- Rejected rejected = new Rejected();
- rejected.setError(ec);
- delivery.disposition(rejected);
- connection.flush();
+ if (store.isRejectingMessages()) {
+ // We drop pre-settled messages (and abort any associated Tx)
+ if (delivery.remotelySettled()) {
+ if (serverSession.getCurrentTransaction() != null) {
+ rollbackCurrentTX(false);
+
+ // Spec requires closure of coordinator link on rejection
of presettled messages within a tx
--- End diff --
The change is spec compliant, certainly, so feel free to merge away if you
want. I was mainly pointing out the comment is imprecise and that of the
options available I think its the less friendly to go with and less likely to
just work.
Clients/applications still cant tell message(s) might be of specific
interest, can't tell which transaction caused a problem if there are multiple
in progress on the link (not that its necessarily currently supported with
Artemis), and have to handle the coordinator link closure and setting up a new
link etc to continue working when using this option, versus the other option
where they just handle the commit success/failure that they would always have
to anyway. Its true that from a time perspective you may find out earlier but
in a lot of cases I don't think that will be significant, even where it is a
long running transaction (since its fairly likely the process that takes a long
time happens before the send that causes an issue).
You're right that it can be said that any clients / applications (depending
on where the exact responsibility for handing the scenario lands in a given
implementation) that don't deal with the scenario well simply aren't
implementing things that the spec says are possible and they should do so.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
