You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@activemq.apache.org by clebertsuconic <gi...@git.apache.org> on 2018/06/20 14:47:16 UTC
[GitHub] activemq-artemis pull request #2152: ARTEMIS-1924 small tweaks on HeartBeat ...
GitHub user clebertsuconic opened a pull request:
https://github.com/apache/activemq-artemis/pull/2152
ARTEMIS-1924 small tweaks on HeartBeat test
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/clebertsuconic/activemq-artemis ARTEMIS-1924
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/activemq-artemis/pull/2152.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 #2152
----
commit d7f6535a2c8d32aee8bef871d986f3a6cc64d641
Author: Clebert Suconic <cl...@...>
Date: 2018-06-20T14:20:42Z
ARTEMIS-1924 small tweaks on HeartBeat test
----
---
[GitHub] activemq-artemis issue #2152: ARTEMIS-1924 small tweaks on HeartBeat test
Posted by gemmellr <gi...@git.apache.org>.
Github user gemmellr commented on the issue:
https://github.com/apache/activemq-artemis/pull/2152
Yes, after a connection-reset-by-peer error which meant the broker knew the connection had gone, which it knew regardless of whether idle-timeout handling was enabled or not, but failed to clear it up at the time. I think we may need to agree to disagree here :)
---
[GitHub] activemq-artemis pull request #2152: ARTEMIS-1924 small tweaks on HeartBeat ...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2152#discussion_r196822722
--- Diff: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/amqp/AmqpNoHearbeatsTest.java ---
@@ -137,18 +139,21 @@ public void inspectOpenedResource(Connection connection) {
}
public static void main(String[] arg) {
- if (arg.length > 0 && arg[0].equals("testConnectionReset")) {
+ if (arg.length > 0 && arg[0].startsWith("testCloseConsumerOnConnectionReset")) {
--- End diff --
I was going to do that, but then the argument was: testCloseConsumerOnConnectionReset[useOverride=true]()
The test is using parameters.. which I didn't like as the name of the queue...
I would prefer leaving it as is...
---
[GitHub] activemq-artemis issue #2152: ARTEMIS-1924 small tweaks on HeartBeat test
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on the issue:
https://github.com/apache/activemq-artemis/pull/2152
@gemmellr / @mtaylor can you review this?
---
[GitHub] activemq-artemis issue #2152: ARTEMIS-1924 small tweaks on HeartBeat test
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on the issue:
https://github.com/apache/activemq-artemis/pull/2152
with heart beats.. you won't this test.. as ping/pongs will disconnect the client.
Disabling heart beats and still solving issues it's all that this fix was about and the reason I created these tests.
---
[GitHub] activemq-artemis issue #2152: ARTEMIS-1924 small tweaks on HeartBeat test
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on the issue:
https://github.com/apache/activemq-artemis/pull/2152
@gemmellr thanks.. I will merge it then
---
[GitHub] activemq-artemis issue #2152: ARTEMIS-1924 small tweaks on HeartBeat test
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on the issue:
https://github.com/apache/activemq-artemis/pull/2152
@gemmellr that I disagree with.. this is about no heart beat.. and disconnecting the client.
---
[GitHub] activemq-artemis issue #2152: ARTEMIS-1924 small tweaks on HeartBeat test
Posted by gemmellr <gi...@git.apache.org>.
Github user gemmellr commented on the issue:
https://github.com/apache/activemq-artemis/pull/2152
Looks good
---
[GitHub] activemq-artemis issue #2152: ARTEMIS-1924 small tweaks on HeartBeat test
Posted by gemmellr <gi...@git.apache.org>.
Github user gemmellr commented on the issue:
https://github.com/apache/activemq-artemis/pull/2152
To me it seemed more about cleaning up the client regardless of idle-timeout, and that it should do exactly the same if idle-timeout handling is enabled or if it wasn't using AMQP at all.
---
[GitHub] activemq-artemis issue #2152: ARTEMIS-1924 small tweaks on HeartBeat test
Posted by gemmellr <gi...@git.apache.org>.
Github user gemmellr commented on the issue:
https://github.com/apache/activemq-artemis/pull/2152
> with heart beats.. you won't this test.. as ping/pongs will disconnect the client.
The idle-timeout stuff defaults to killing after 60 seconds, which the test doesnt even allow for running long enough to occur, so disabling heartbeats should make no difference to the test, which is why I dont think it should be in this class. But I agree, lets move on.
---
[GitHub] activemq-artemis issue #2152: ARTEMIS-1924 small tweaks on HeartBeat test
Posted by gemmellr <gi...@git.apache.org>.
Github user gemmellr commented on the issue:
https://github.com/apache/activemq-artemis/pull/2152
(I still don't think it should be in that class though per #2149 comments...but the test itself looks good now)
---
[GitHub] activemq-artemis issue #2152: ARTEMIS-1924 small tweaks on HeartBeat test
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on the issue:
https://github.com/apache/activemq-artemis/pull/2152
@gemmellr the issue i was dealing with was a .NET client in windows not cleaning up after a disconnect, after I created the feature. So.. this is definitely related. We can create more tests outside of this context.. but this test is definitely needed IMO.
---
[GitHub] activemq-artemis issue #2152: ARTEMIS-1924 small tweaks on HeartBeat test
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on the issue:
https://github.com/apache/activemq-artemis/pull/2152
It's pointless to argue anyway.. it's just a test and it's working as expected... I need to move to a different task ;)
---
[GitHub] activemq-artemis pull request #2152: ARTEMIS-1924 small tweaks on HeartBeat ...
Posted by gemmellr <gi...@git.apache.org>.
Github user gemmellr commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2152#discussion_r196818582
--- Diff: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/amqp/AmqpNoHearbeatsTest.java ---
@@ -137,18 +139,21 @@ public void inspectOpenedResource(Connection connection) {
}
public static void main(String[] arg) {
- if (arg.length > 0 && arg[0].equals("testConnectionReset")) {
+ if (arg.length > 0 && arg[0].startsWith("testCloseConsumerOnConnectionReset")) {
--- End diff --
How about using the argument as the queue name? That way you dont need a constant, and there are side benefits such as test isolation and if things to wonky you can read the logs and immediately know what test the output came from etc?
---
[GitHub] activemq-artemis pull request #2152: ARTEMIS-1924 small tweaks on HeartBeat ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/activemq-artemis/pull/2152
---