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 2017/06/06 16:59:41 UTC
[GitHub] activemq-artemis pull request #1321: ARTEMIS-1206 SetClientID from Core Conn...
GitHub user clebertsuconic opened a pull request:
https://github.com/apache/activemq-artemis/pull/1321
ARTEMIS-1206 SetClientID from Core ConnectionFactory is not unique
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/clebertsuconic/activemq-artemis ARTEMIS-1206
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/activemq-artemis/pull/1321.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 #1321
----
commit 8368f410436e68a48b53213ea5a98f5b82f3b00d
Author: Clebert Suconic <cl...@apache.org>
Date: 2017-06-06T16:58:28Z
ARTEMIS-1206 SetClientID from Core ConnectionFactory is not unique
----
---
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 #1321: ARTEMIS-1206 SetClientID from Core ConnectionF...
Posted by tabish121 <gi...@git.apache.org>.
Github user tabish121 commented on the issue:
https://github.com/apache/activemq-artemis/pull/1321
It is indeed validating the same property, that property is the SOLE_CONTAINER capability that is sent in the open frame to indicate the uniqueness of the client ID as being required.
What you are testing is the Qpid JMS client specific implementation of the setClientID and how it controls when the open frame is sent. What you aren't testing in the new test is the affect of not specifying a client ID and also the behavior of setClientID through the Connection or no client ID being set and calling connection start.
So if you feel you need this particular test case then for completeness I'd argue you test all variations of client ID / no client ID and ConnectionFactory vs Connection setClientID.
---
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 #1321: ARTEMIS-1206 SetClientID from Core ConnectionF...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on the issue:
https://github.com/apache/activemq-artemis/pull/1321
The test you showed doesn't seem to be using it throught the connection factory property. I don't think it hurts a small extra 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 #1321: ARTEMIS-1206 SetClientID from Core ConnectionF...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on the issue:
https://github.com/apache/activemq-artemis/pull/1321
duh, you're right.. ammending
---
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 #1321: ARTEMIS-1206 SetClientID from Core ConnectionF...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on the issue:
https://github.com/apache/activemq-artemis/pull/1321
@tabish121 fixed JORAM... your other test is not validating the same property through the CF. I would leave it there just in case.. it's a fairly simple test.. it doesn't hurt.
---
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 #1321: ARTEMIS-1206 SetClientID from Core ConnectionF...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on the issue:
https://github.com/apache/activemq-artemis/pull/1321
@tabish121 @gemmellr fair enough.. test removed.
---
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 #1321: ARTEMIS-1206 SetClientID from Core ConnectionF...
Posted by tabish121 <gi...@git.apache.org>.
Github user tabish121 commented on the issue:
https://github.com/apache/activemq-artemis/pull/1321
Test Qpid JMS based version of the test is actually also not necessary as what you are testing there is that the broker is respecting the AMQP JMS Mapping defined SOLE_CONTAINER capability which is already tested here: https://github.com/apache/activemq-artemis/blob/master/tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/amqp/AmqpInboundConnectionTest.java#L201
The JMS client tests its own implementation of the setClientID etc methods.
---
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 #1321: ARTEMIS-1206 SetClientID from Core ConnectionF...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on the issue:
https://github.com/apache/activemq-artemis/pull/1321
^^^ This is why PRs are good... everyone is entitled to make something stoopid.. this gives another chance to be caught 👍
---
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 #1321: ARTEMIS-1206 SetClientID from Core ConnectionF...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on the issue:
https://github.com/apache/activemq-artemis/pull/1321
will have to fix the JORAM 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 #1321: ARTEMIS-1206 SetClientID from Core ConnectionF...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on the issue:
https://github.com/apache/activemq-artemis/pull/1321
@tabish121 I'm just adding the same case where it failed on core JMS. Just wanted to make sure the property from the Connection Factory is also applied to the connection the same way. I think that's a valid test.. it didn't fail but I don't see a reason to remove it.
---
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 #1321: ARTEMIS-1206 SetClientID from Core ConnectionF...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on the issue:
https://github.com/apache/activemq-artemis/pull/1321
@tabish121 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 #1321: ARTEMIS-1206 SetClientID from Core Conn...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/activemq-artemis/pull/1321
---
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 #1321: ARTEMIS-1206 SetClientID from Core ConnectionF...
Posted by tabish121 <gi...@git.apache.org>.
Github user tabish121 commented on the issue:
https://github.com/apache/activemq-artemis/pull/1321
Seems a bit odd that core JMS client functionality is being tested in the AMQP integration tests package, I'd suggest that the AMQP tests belong here and tests for the core JMS client belong in whatever area that client is already being tested in, otherwise it is hard to later on find the core client coverage 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 #1321: ARTEMIS-1206 SetClientID from Core ConnectionF...
Posted by gemmellr <gi...@git.apache.org>.
Github user gemmellr commented on the issue:
https://github.com/apache/activemq-artemis/pull/1321
I don't think Tim is saying its not valid so much as it is essentially just a client test in this situation (and one of several that could exist), with the brokers required AMQP behaviour already verified by more explicit testing around the related SOLE_CONTAINER capability that the AMQP JMS client uses to achieve its desired behaviour, making it unclear why you would really add it to the brokers test suite when from the brokers perspective its basically just increasing the length of the test suite. I'd say the same personally.
---
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.
---