You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@activemq.apache.org by jdanekrh <gi...@git.apache.org> on 2017/07/18 20:54:01 UTC
[GitHub] activemq-artemis pull request #1407: ARTEMIS-1276 fix JmsSendReceiveWithMess...
GitHub user jdanekrh opened a pull request:
https://github.com/apache/activemq-artemis/pull/1407
ARTEMIS-1276 fix JmsSendReceiveWithMessageExpirationTest [wip]
The JmsSendReceiveWithMessageExpirationTest#testConsumeExpiredQueueAndDlq test used to fail with
> JmsQueueCompositeSendReceiveTest>CombinationTestSupport.runBare:144->CombinationTestSupport.runBare:138->testSendReceive:86->JmsSendReceiveTestSupport.testSendReceive:
> 115->JmsSendReceiveTestSupport.assertMessagesAreReceived:156->JmsSendReceiveTestSupport.assertMessagesReceivedAreValid:176 Invalid number of messages received
> expected:<100> but was:<0>
After the changes in this PR, it is now failing with
junit.framework.AssertionFailedError: got dlq messages
Expected :99
Actual :90
I can get it to pass if I disable prefetch, that is, I also change
```
diff --git a/tests/activemq5-unit-tests/src/test/java/org/apache/activemq/JmsSendReceiveWithMessageExpirationTest.java b/tests/activemq5-unit-tests/src/test/java/org/apache/activemq/JmsSendReceiveWithMessageExpirationTest.java
index dffd72296..3ff305810 100644
--- a/tests/activemq5-unit-tests/src/test/java/org/apache/activemq/JmsSendReceiveWithMessageExpirationTest.java
+++ b/tests/activemq5-unit-tests/src/test/java/org/apache/activemq/JmsSendReceiveWithMessageExpirationTest.java
@@ -126,7 +126,7 @@ public class JmsSendReceiveWithMessageExpirationTest extends TestSupport {
Connection consumerConnection = createConnection();
ActiveMQPrefetchPolicy prefetchPolicy = new ActiveMQPrefetchPolicy();
- prefetchPolicy.setAll(10);
+ prefetchPolicy.setAll(0);
((ActiveMQConnection) consumerConnection).setPrefetchPolicy(prefetchPolicy);
Session consumerSession = consumerConnection.createSession(false, Session.CLIENT_ACKNOWLEDGE);
MessageConsumer consumer = consumerSession.createConsumer(consumerDestination);
```
I do not understand why prefetch breaks the test, and I do not know how to begin investigating this effectively...
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/jdanekrh/activemq-artemis jd_JmsSendReceiveWithMessageExpirationTest
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/activemq-artemis/pull/1407.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1407
----
commit b629c1b2835af5e60593ec6c79adce83d7962ece
Author: Jiri Danek <jd...@redhat.com>
Date: 2017-06-26T13:16:44Z
ARTEMIS-1276 fix JmsSendReceiveWithMessageExpirationTest
----
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis issue #1407: ARTEMIS-1276 fix JmsSendReceiveWithMessageExpi...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on the issue:
https://github.com/apache/activemq-artemis/pull/1407
it's probably an issue on the test.
should I merge this already?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis issue #1407: ARTEMIS-1276 fix JmsSendReceiveWithMessageExpi...
Posted by jdanekrh <gi...@git.apache.org>.
Github user jdanekrh commented on the issue:
https://github.com/apache/activemq-artemis/pull/1407
The only change to test is adding line `commonSettings.setExpiryAddress(dla);` in broker creation. That causes the test to progress further (passes assert) and then the test tries to cast `Broker` it got from calling `getRegionBroker` to `RegionBroker`, so that had to be implemented (using mockito). And then it looks at statistics (messages in flight) so then I had to write a wrapper/fake/whatever-it-should-be-called for that.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis issue #1407: ARTEMIS-1276 fix JmsSendReceiveWithMessageExpi...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on the issue:
https://github.com/apache/activemq-artemis/pull/1407
@jdanekrh sure, but I'm wondering if this test should be left alone. perhaps just fixing that would be enough to fix this test?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis issue #1407: ARTEMIS-1276 fix JmsSendReceiveWithMessageExpi...
Posted by jdanekrh <gi...@git.apache.org>.
Github user jdanekrh commented on the issue:
https://github.com/apache/activemq-artemis/pull/1407
> messages only expire on client on activemq5 while it's on server & client on artemis
Did you mean expire on _server_ on activemq5? If it expired on client, how would it then get to DLQ queue on server?
---
I think get it now. When the test receives for the first time (first `for` loop), the client gets first 10 messages sent, which are those with short expiration time. One of them is delivered ok, the 9 remaining expire on the client while it is in sleep(). That means these 9 never get into the DLQ, because for that they would have to expire on the server. Which explains the `actual: 90, expected: 99` result when it is then receiving from DLQ.
I did not know about this difference between 5 and Artemis.
From user point of view, the fact that prefetch changes behavior (and is not just performance optimization) and that it can cause what can be thought of as a message loss (these 9 messages have irrevocably disappeared, right?) could be considered a bug, I think.
I guess there is nothing to be done about it here, so removing [wip].
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis issue #1407: ARTEMIS-1276 fix JmsSendReceiveWithMessageExpi...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on the issue:
https://github.com/apache/activemq-artemis/pull/1407
>>> If it expired on client, how would it then get to DLQ queue on server?
There's a special protocol on OpenWire to tell the server the message expired. On Artemis we would just reject the message and let the server deal with it again.
Having said that, perhaps there's something wrong on treating that message..so it's worth to check if it's a bug on that area.. or just a test issue.
I didn't look into the details specifically to this test BTW: I was just mentioning that I had cases like this where I had to tweak the test because of that semantic change.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis pull request #1407: ARTEMIS-1276 fix JmsSendReceiveWithMess...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/activemq-artemis/pull/1407
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis issue #1407: ARTEMIS-1276 fix JmsSendReceiveWithMessageExpi...
Posted by jdanekrh <gi...@git.apache.org>.
Github user jdanekrh commented on the issue:
https://github.com/apache/activemq-artemis/pull/1407
> it's probably an issue on the test.
I was not sure. The same test is passing with ActiveMQ 5.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis issue #1407: ARTEMIS-1276 fix JmsSendReceiveWithMessageExpi...
Posted by jdanekrh <gi...@git.apache.org>.
Github user jdanekrh commented on the issue:
https://github.com/apache/activemq-artemis/pull/1407
I'd prefer to put the fix into a new PR, if you don't mind. I think there is already enough things in this one already.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis issue #1407: ARTEMIS-1276 fix JmsSendReceiveWithMessageExpi...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on the issue:
https://github.com/apache/activemq-artemis/pull/1407
@jdanekrh if it's a bug on these acks, then lets kep this test the way it is then... or ammend this PR with this fix you mentioned instead. I think it's right.. we just need to run the whole testsuite on openwire and integration-tests.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis issue #1407: ARTEMIS-1276 fix JmsSendReceiveWithMessageExpi...
Posted by jdanekrh <gi...@git.apache.org>.
Github user jdanekrh commented on the issue:
https://github.com/apache/activemq-artemis/pull/1407
Right. Yeah, I think this one is actual OpenWire handling bug in Artemis. If the message expires while it is being prefetched on client, server drops it and never tries to expire it.
i may even have a fix, but this message lifecycle is quite complicated, so it is likely I am missing something...
```diff
--- a/artemis-protocols/artemis-openwire-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/openwire/amq/AMQConsumer.java
+++ b/artemis-protocols/artemis-openwire-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/openwire/amq/AMQConsumer.java
@@ -298,8 +298,9 @@ public class AMQConsumer {
}
}
if (ack.isExpiredAck()) {
- //adjust delivering count for expired messages
- this.serverConsumer.getQueue().decDelivering(ackList.size());
+ for (MessageReference ref : ackList) {
+ ref.getQueue().expire(ref);
+ }
}
}
```
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis issue #1407: ARTEMIS-1276 fix JmsSendReceiveWithMessageExpi...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on the issue:
https://github.com/apache/activemq-artemis/pull/1407
the test would take the consideration the messages only expire on client on activemq5 while it's on server & client on artemis. I had seen some other tests failing because of that.. wrong assertions.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---