You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@activemq.apache.org by cshannon <gi...@git.apache.org> on 2018/05/30 12:52:19 UTC
[GitHub] activemq-artemis pull request #2116: ARTEMIS-1895 - Add duplicate metadata f...
GitHub user cshannon opened a pull request:
https://github.com/apache/activemq-artemis/pull/2116
ARTEMIS-1895 - Add duplicate metadata failure callback to ActiveMQServerPlugin
Add a callback on duplicate metadata which will allow extra functionality to be added.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/cshannon/activemq-artemis ARTEMIS-1895
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/activemq-artemis/pull/2116.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 #2116
----
commit afba4982b661759b3e7f1c6a2e4095fbc4c5a1dd
Author: Christopher L. Shannon (cshannon) <ch...@...>
Date: 2018-05-30T12:50:05Z
ARTEMIS-1895 - Add duplicate metadata failure callback to ActiveMQServerPlugin
Add a callback on duplicate metadata which will allow extra
functionality to be added.
----
---
[GitHub] activemq-artemis pull request #2116: ARTEMIS-1895 - Add duplicate metadata f...
Posted by cshannon <gi...@git.apache.org>.
Github user cshannon commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2116#discussion_r191788973
--- Diff: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/jms/client/SessionMetadataAddExceptionTest.java ---
@@ -75,6 +88,24 @@ public void testInvalidClientIdSetFactory() throws Exception {
cf.createConnection();
}
+ @Test(timeout = 5000, expected = JMSException.class)
+ public void testDuplicateClientIdSet() throws Exception {
+ ActiveMQConnectionFactory activeMQConnectionFactory = (ActiveMQConnectionFactory) cf;
+ Connection con = cf.createConnection();
+ Connection con2 = cf.createConnection();
+ try {
+ con.setClientID("valid");
+ con2.setClientID("valid");
+ fail("Should have failed for duplicate clientId");
+ } catch (Exception e) {
--- End diff --
I also added another test to verify that ActiveMQException bubbles up and is converted to a JMSException by the client
---
[GitHub] activemq-artemis pull request #2116: ARTEMIS-1895 - Add duplicate metadata f...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2116#discussion_r191783207
--- Diff: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/jms/client/SessionMetadataAddExceptionTest.java ---
@@ -75,6 +88,24 @@ public void testInvalidClientIdSetFactory() throws Exception {
cf.createConnection();
}
+ @Test(timeout = 5000, expected = JMSException.class)
+ public void testDuplicateClientIdSet() throws Exception {
+ ActiveMQConnectionFactory activeMQConnectionFactory = (ActiveMQConnectionFactory) cf;
+ Connection con = cf.createConnection();
+ Connection con2 = cf.createConnection();
+ try {
+ con.setClientID("valid");
+ con2.setClientID("valid");
+ fail("Should have failed for duplicate clientId");
+ } catch (Exception e) {
--- End diff --
Should this test that explicit exception type is thrown not just any old exception. As probably should fail if an unexpectes exception is ever thrown.
---
[GitHub] activemq-artemis issue #2116: ARTEMIS-1895 - Add duplicate metadata failure ...
Posted by cshannon <gi...@git.apache.org>.
Github user cshannon commented on the issue:
https://github.com/apache/activemq-artemis/pull/2116
@clebertsuconic - is this ok to merge?
---
[GitHub] activemq-artemis issue #2116: ARTEMIS-1895 - Add duplicate metadata failure ...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/2116
Looks good to me
---
[GitHub] activemq-artemis pull request #2116: ARTEMIS-1895 - Add duplicate metadata f...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2116#discussion_r191793991
--- Diff: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/jms/client/SessionMetadataAddExceptionTest.java ---
@@ -75,6 +93,37 @@ public void testInvalidClientIdSetFactory() throws Exception {
cf.createConnection();
}
+ @Test(timeout = 5000, expected = JMSException.class)
+ public void testDuplicateClientIdSet() throws Exception {
+ ActiveMQConnectionFactory activeMQConnectionFactory = (ActiveMQConnectionFactory) cf;
+ Connection con = cf.createConnection();
+ Connection con2 = cf.createConnection();
+ try {
+ con.setClientID("valid");
+ con2.setClientID("valid");
+ fail("Should have failed for duplicate clientId");
+ } catch (InvalidClientIDException e) {
+ assertEquals(1, duplicateCount.get());
+ throw e;
--- End diff --
Problem here is assert will be bypassed and test will still pass if another exception thrown as tests expects JMSException.
Expects should be explicit and need a catch all with and assert fail
---
[GitHub] activemq-artemis pull request #2116: ARTEMIS-1895 - Add duplicate metadata f...
Posted by cshannon <gi...@git.apache.org>.
Github user cshannon commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2116#discussion_r191787441
--- Diff: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/jms/client/SessionMetadataAddExceptionTest.java ---
@@ -75,6 +88,24 @@ public void testInvalidClientIdSetFactory() throws Exception {
cf.createConnection();
}
+ @Test(timeout = 5000, expected = JMSException.class)
+ public void testDuplicateClientIdSet() throws Exception {
+ ActiveMQConnectionFactory activeMQConnectionFactory = (ActiveMQConnectionFactory) cf;
+ Connection con = cf.createConnection();
+ Connection con2 = cf.createConnection();
+ try {
+ con.setClientID("valid");
+ con2.setClientID("valid");
+ fail("Should have failed for duplicate clientId");
+ } catch (Exception e) {
--- End diff --
I switched it to catch the InvalidClientIdException that gets thrown
---
[GitHub] activemq-artemis issue #2116: ARTEMIS-1895 - Add duplicate metadata failure ...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on the issue:
https://github.com/apache/activemq-artemis/pull/2116
The easiest way to rebuild is to.
Got commit —amend
Got push -f
---
[GitHub] activemq-artemis issue #2116: ARTEMIS-1895 - Add duplicate metadata failure ...
Posted by cshannon <gi...@git.apache.org>.
Github user cshannon commented on the issue:
https://github.com/apache/activemq-artemis/pull/2116
I don't think it's related as the test it failed on doesn't have a plugin registered. I tried to see if I could trigger it to re-run the build in travis ci but i didn't see a way to do that.
---
[GitHub] activemq-artemis issue #2116: ARTEMIS-1895 - Add duplicate metadata failure ...
Posted by cshannon <gi...@git.apache.org>.
Github user cshannon commented on the issue:
https://github.com/apache/activemq-artemis/pull/2116
The build looks good now
---
[GitHub] activemq-artemis issue #2116: ARTEMIS-1895 - Add duplicate metadata failure ...
Posted by cshannon <gi...@git.apache.org>.
Github user cshannon commented on the issue:
https://github.com/apache/activemq-artemis/pull/2116
I removed the re-throw and the expect for the JMSException so it will only pass on InvalidClientIDException as that is the only type that should be thrown
---
[GitHub] activemq-artemis pull request #2116: ARTEMIS-1895 - Add duplicate metadata f...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2116#discussion_r191777420
--- Diff: artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQConnection.java ---
@@ -263,6 +264,8 @@ private void validateClientID(ClientSession validateSession, String clientID) th
} catch (ActiveMQException e) {
if (e.getType() == ActiveMQExceptionType.DUPLICATE_METADATA) {
throw new InvalidClientIDException("clientID=" + clientID + " was already set into another connection");
+ } else {
+ throw new InvalidClientIDException("Error setting clientID=" + clientID + ": " + e.getMessage());
--- End diff --
The original exception here may not be related to InvalidClientId, seems a bit dangerous to catchall and throw like this. Maybe bubble up or convert the exception
---
[GitHub] activemq-artemis issue #2116: ARTEMIS-1895 - Add duplicate metadata failure ...
Posted by cshannon <gi...@git.apache.org>.
Github user cshannon commented on the issue:
https://github.com/apache/activemq-artemis/pull/2116
@michaelandrepearce - good point, i fixed it to bubble up the ActiveMQException and so it is handled properly by the client
---
[GitHub] activemq-artemis issue #2116: ARTEMIS-1895 - Add duplicate metadata failure ...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on the issue:
https://github.com/apache/activemq-artemis/pull/2116
thanks a lot.. merging
---
[GitHub] activemq-artemis pull request #2116: ARTEMIS-1895 - Add duplicate metadata f...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/activemq-artemis/pull/2116
---