You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@activemq.apache.org by michaelandrepearce <gi...@git.apache.org> on 2017/06/06 08:54:59 UTC
[GitHub] activemq-artemis pull request #1317: ARTEMIS-1207: Align order of when setCl...
GitHub user michaelandrepearce opened a pull request:
https://github.com/apache/activemq-artemis/pull/1317
ARTEMIS-1207: Align order of when setClientId can be called with AcitveMQ5 and QPID
Update ActiveMQConnection to change behaviour
Add test to avoid regression.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/michaelandrepearce/activemq-artemis ARTEMIS-1207
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/activemq-artemis/pull/1317.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 #1317
----
commit 1f09a8e3046ed2769a9a80e7851e2e9ab56041a9
Author: Michael Andre Pearce <mi...@me.com>
Date: 2017-06-06T08:54:02Z
ARTEMIS-1207: [Core JMS Client] Align order of when setClientId can be called with AcitveMQ5 and QPID
Update ActiveMQConnection to change behaviour
Add test to avoid regression.
----
---
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 #1317: ARTEMIS-1207: Align order of when setClientId ...
Posted by jbertram <gi...@git.apache.org>.
Github user jbertram commented on the issue:
https://github.com/apache/activemq-artemis/pull/1317
What about the other methods mentioned in the JIRA (i.e. getMetaData and stop)?
---
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 #1317: ARTEMIS-1207: Align order of when setCl...
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/1317#discussion_r120375080
--- Diff: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/jms/connection/ExceptionListenerTest.java ---
@@ -143,4 +143,22 @@ public void testListenerCalledForOneConnectionAndSessions() throws Exception {
conn.close();
}
+
+ /**
+ * The JMS Spec isn't specific about if ClientId can be set after Exception Listener or not,
+ * simply it states that clientId must be set before any operation (read as remote)
+ *
+ * QpidJMS and ActiveMQ5 both interpret that therefor you can set the exception lister first.
+ * As such we align with those, allowing the exception listener to be set prior to the clientId,
+ * This to avoid causing implementation nuance's, when switching code from one client to another.
+ *
+ * This test is to test this and to ensure it doesn't get accidentally regressed.
+ */
+ @Test
+ public void testSetClientIdAfterSetExceptionListener() throws Exception {
+ Connection conn = cf.createConnection();
--- End diff --
You are not using getExceptionListener here.. is the test really duplicating the issue?
---
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 #1317: ARTEMIS-1207: Align order of when setCl...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/activemq-artemis/pull/1317
---
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 #1317: ARTEMIS-1207: Align order of when setClientId ...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/1317
@jbertram i only just see the update in the JIRA ticket, sure we can add these also. I can work on aligning those also. You ok to merge this one, whilst i work on those also? We close JIRA only once all done?
---
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 #1317: ARTEMIS-1207: Align order of when setCl...
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/1317#discussion_r120378248
--- Diff: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/jms/connection/ExceptionListenerTest.java ---
@@ -143,4 +143,22 @@ public void testListenerCalledForOneConnectionAndSessions() throws Exception {
conn.close();
}
+
+ /**
+ * The JMS Spec isn't specific about if ClientId can be set after Exception Listener or not,
+ * simply it states that clientId must be set before any operation (read as remote)
+ *
+ * QpidJMS and ActiveMQ5 both interpret that therefor you can set the exception lister first.
+ * As such we align with those, allowing the exception listener to be set prior to the clientId,
+ * This to avoid causing implementation nuance's, when switching code from one client to another.
+ *
+ * This test is to test this and to ensure it doesn't get accidentally regressed.
+ */
+ @Test
+ public void testSetClientIdAfterSetExceptionListener() throws Exception {
+ Connection conn = cf.createConnection();
--- End diff --
I thought, from what I read the code, the issue was calling setExceptionListener and clientID wouldn't fail.. I'm confused on what is the issue now..
---
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 #1317: ARTEMIS-1207: Align order of when setClientId ...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/1317
Will work on this tonight / train home, hopefully.
---
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 #1317: ARTEMIS-1207: Align order of when setCl...
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/1317#discussion_r120379443
--- Diff: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/jms/connection/ExceptionListenerTest.java ---
@@ -143,4 +143,22 @@ public void testListenerCalledForOneConnectionAndSessions() throws Exception {
conn.close();
}
+
+ /**
+ * The JMS Spec isn't specific about if ClientId can be set after Exception Listener or not,
+ * simply it states that clientId must be set before any operation (read as remote)
+ *
+ * QpidJMS and ActiveMQ5 both interpret that therefor you can set the exception lister first.
+ * As such we align with those, allowing the exception listener to be set prior to the clientId,
+ * This to avoid causing implementation nuance's, when switching code from one client to another.
+ *
+ * This test is to test this and to ensure it doesn't get accidentally regressed.
+ */
+ @Test
+ public void testSetClientIdAfterSetExceptionListener() throws Exception {
+ Connection conn = cf.createConnection();
--- End diff --
the issue was if you setExceptionListener before clientId, it would fail/throw exceptions on setting clientId. Now it is possible to set it before setting the clientId, this test is testing this behaviour. (e.g. if you revert the change in the ActiveMQConnection , this test fails)
---
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 #1317: ARTEMIS-1207: Align order of when setCl...
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/1317#discussion_r120377162
--- Diff: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/jms/connection/ExceptionListenerTest.java ---
@@ -143,4 +143,22 @@ public void testListenerCalledForOneConnectionAndSessions() throws Exception {
conn.close();
}
+
+ /**
+ * The JMS Spec isn't specific about if ClientId can be set after Exception Listener or not,
+ * simply it states that clientId must be set before any operation (read as remote)
+ *
+ * QpidJMS and ActiveMQ5 both interpret that therefor you can set the exception lister first.
+ * As such we align with those, allowing the exception listener to be set prior to the clientId,
+ * This to avoid causing implementation nuance's, when switching code from one client to another.
+ *
+ * This test is to test this and to ensure it doesn't get accidentally regressed.
+ */
+ @Test
+ public void testSetClientIdAfterSetExceptionListener() throws Exception {
+ Connection conn = cf.createConnection();
--- End diff --
the issue was setExceptionListener that we were hitting, but you're correct getExceptionListener should be the same, we should add that test, will add.
---
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 #1317: ARTEMIS-1207: Align order of when setClientId ...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on the issue:
https://github.com/apache/activemq-artemis/pull/1317
@michaelandrepearce I'm ok with that.. keep the JIRA open
---
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.
---