You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@activemq.apache.org by "gtully (via GitHub)" <gi...@apache.org> on 2023/08/30 17:28:12 UTC
[GitHub] [activemq-artemis] gtully opened a new pull request, #4597: ARTEMIS-4410 - for exclusive queues, gate dispatch on no outstanding …
gtully opened a new pull request, #4597:
URL: https://github.com/apache/activemq-artemis/pull/4597
…deliveries
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gtully commented on pull request #4597: ARTEMIS-4410 - for exclusive queues, gate dispatch on no outstanding …
Posted by "gtully (via GitHub)" <gi...@apache.org>.
gtully commented on PR #4597:
URL: https://github.com/apache/activemq-artemis/pull/4597#issuecomment-1700780673
the canDispatch gate logic looks brittle, a simpler solution as suggested by @gemmellr is to leave the exclusive consumer in place as BUSY till it remove context has completed. Will investigate a fix with that approach.
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gemmellr commented on a diff in pull request #4597: ARTEMIS-4410 - for exclusive queues, gate dispatch on no outstanding …
Posted by "gemmellr (via GitHub)" <gi...@apache.org>.
gemmellr commented on code in PR #4597:
URL: https://github.com/apache/activemq-artemis/pull/4597#discussion_r1311416254
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/QueueImpl.java:
##########
@@ -1434,6 +1436,10 @@ private boolean canDispatch() {
return false;
}
+ if (isExclusive() && hasExclusivePendingDeliveries()) {
+ return false;
+ }
Review Comment:
(Sometimes maybe, but overall no due to the earlier-and-later 'remember can dispatch' logic I had overlooked)
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gtully commented on pull request #4597: ARTEMIS-4410 - for exclusive queues, gate dispatch on no outstanding …
Posted by "gtully (via GitHub)" <gi...@apache.org>.
gtully commented on PR #4597:
URL: https://github.com/apache/activemq-artemis/pull/4597#issuecomment-1700599316
full test suite looks good.
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gtully commented on pull request #4597: ARTEMIS-4410 - for exclusive queues, gate dispatch on no outstanding …
Posted by "gtully (via GitHub)" <gi...@apache.org>.
gtully commented on PR #4597:
URL: https://github.com/apache/activemq-artemis/pull/4597#issuecomment-1699533685
want to see what the test suite says and need some validation that I have not missed any countDown! will remove draft status once validate a full test suite run. but any eyes in the mean time are welcome :-)
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gtully commented on pull request #4597: ARTEMIS-4410 - for exclusive queues, gate dispatch on no outstanding …
Posted by "gtully (via GitHub)" <gi...@apache.org>.
gtully commented on PR #4597:
URL: https://github.com/apache/activemq-artemis/pull/4597#issuecomment-1700656812
I think I see a logic error, investigating - back to draft
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gemmellr merged pull request #4597: ARTEMIS-4410 - process delivery cancellations before removing [exclusive] consumer
Posted by "gemmellr (via GitHub)" <gi...@apache.org>.
gemmellr merged PR #4597:
URL: https://github.com/apache/activemq-artemis/pull/4597
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gemmellr commented on a diff in pull request #4597: ARTEMIS-4410 - for exclusive queues, gate dispatch on no outstanding …
Posted by "gemmellr (via GitHub)" <gi...@apache.org>.
gemmellr commented on code in PR #4597:
URL: https://github.com/apache/activemq-artemis/pull/4597#discussion_r1311338687
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/QueueImpl.java:
##########
@@ -1434,6 +1436,10 @@ private boolean canDispatch() {
return false;
}
+ if (isExclusive() && hasExclusivePendingDeliveries()) {
+ return false;
+ }
Review Comment:
Doesnt this mean it will only send 1 outstanding message at a time? That doesnt seem desirable?
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gtully commented on pull request #4597: ARTEMIS-4410 - for exclusive queues, gate dispatch on no outstanding …
Posted by "gtully (via GitHub)" <gi...@apache.org>.
gtully commented on PR #4597:
URL: https://github.com/apache/activemq-artemis/pull/4597#issuecomment-1700903836
good observation.
there are only two calls to org.apache.activemq.artemis.core.server.impl.ServerConsumerImpl#cancelRefs one in a session where the started flags is flipped for this same ordering issue.
the other is the close (in the connection error case), where i see started is already flipped.
I think it may be a case of just deferring the remove till after the cancel in the close.
when !started a server consumer reports BUSY!
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on pull request #4597: ARTEMIS-4410 - for exclusive queues, gate dispatch on no outstanding …
Posted by "clebertsuconic (via GitHub)" <gi...@apache.org>.
clebertsuconic commented on PR #4597:
URL: https://github.com/apache/activemq-artemis/pull/4597#issuecomment-1701207073
the fix I see now seems very simple. if test suite is good, we should merge 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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org