You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@activemq.apache.org by jbertram <gi...@git.apache.org> on 2018/01/31 13:48:46 UTC
[GitHub] activemq-artemis pull request #1830: ARTEMIS-1644 legacy clients can't acces...
GitHub user jbertram opened a pull request:
https://github.com/apache/activemq-artemis/pull/1830
ARTEMIS-1644 legacy clients can't access resources with old prefixes
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/jbertram/activemq-artemis ARTEMIS-1644
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/activemq-artemis/pull/1830.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 #1830
----
commit 20928c464e23ba93038bd96915767c7ead5bd856
Author: Justin Bertram <jb...@...>
Date: 2018-01-30T19:04:51Z
ARTEMIS-1644 legacy clients can't access resources with old prefixes
----
---
[GitHub] activemq-artemis pull request #1830: ARTEMIS-1644 legacy clients can't acces...
Posted by mtaylor <gi...@git.apache.org>.
Github user mtaylor commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1830#discussion_r165070460
--- Diff: artemis-core-client/src/main/java/org/apache/activemq/artemis/core/protocol/core/impl/wireformat/QueueAbstractPacket.java ---
@@ -69,30 +49,29 @@ public SimpleString getAddress(int clientVersion) {
*/
public final List<SimpleString> convertQueueNames(int clientVersion, List<SimpleString> queueNames) {
if (clientVersion < ADDRESSING_CHANGE_VERSION) {
- return applyAddressPrefixTo(queueNames);
- } else {
- return queueNames;
- }
- }
-
- private List<SimpleString> applyAddressPrefixTo(List<SimpleString> queueNames) {
- final int names = queueNames.size();
- if (names == 0) {
- return Collections.emptyList();
- } else {
- final SimpleString address = this.address;
- final SimpleString prefix = jmsPrefixOf(address);
- if (prefix != null) {
- final List<SimpleString> prefixedQueueNames = new ArrayList<>(names);
- for (int i = 0; i < names; i++) {
- final SimpleString oldQueueNames = queueNames.get(i);
- final SimpleString prefixedQueueName = prefix.concat(oldQueueNames);
- prefixedQueueNames.add(prefixedQueueName);
- }
- return prefixedQueueNames;
+ final int names = queueNames.size();
--- End diff --
@jbertram I am confused by this. Why are you still checking for Client Verison < ADDRESSING_CHANGE_VERSION?
The point of the JIRA is to allow old clients to continue using the old address space. So, I should be able to configure a queue called "jms.queue.foo" and have the old client connect to it. It should not matter what client or protocol you are using. For this reason, we do not want to strip any prefix in the default case. I.e. there is no prefix set on the acceptor (regardless of the client version).
We only need to configure an acceptor when using the old client and the new address model. In that case, you only need to check to see if there's a prefix enabled, not the client version. This is a generic feature across all protocols.
The key thing is to allow the old client to continue using the old address space. By using the generic feature.
---
[GitHub] activemq-artemis pull request #1830: ARTEMIS-1644 legacy clients can't acces...
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/1830#discussion_r165077949
--- Diff: artemis-cli/src/main/resources/org/apache/activemq/artemis/cli/commands/etc/broker.xml ---
@@ -78,7 +78,7 @@ ${global-max-section}
<!-- amqpLowCredits: The server will send the # credits specified at amqpCredits at this low mark -->
<!-- Acceptor for every supported protocol -->
- <acceptor name="artemis">tcp://${host}:${default.port}?tcpSendBufferSize=1048576;tcpReceiveBufferSize=1048576;protocols=CORE,AMQP,STOMP,HORNETQ,MQTT,OPENWIRE;useEpoll=true;amqpCredits=1000;amqpLowCredits=300</acceptor>
+ <acceptor name="artemis">tcp://${host}:${default.port}?anycastPrefix=jms.queue.;multicastPrefix=jms.topic.;tcpSendBufferSize=1048576;tcpReceiveBufferSize=1048576;protocols=CORE,AMQP,STOMP,HORNETQ,MQTT,OPENWIRE;useEpoll=true;amqpCredits=1000;amqpLowCredits=300</acceptor>
--- End diff --
@jbertram we could have this commented out... like.. if you have old acceptors. use this configuration...
but we can't have this by default.
We can change the compatiblity tests to add prefixes and make sure they work together.
---
[GitHub] activemq-artemis pull request #1830: ARTEMIS-1644 legacy clients can't acces...
Posted by jbertram <gi...@git.apache.org>.
Github user jbertram commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1830#discussion_r165077778
--- Diff: artemis-cli/src/main/resources/org/apache/activemq/artemis/cli/commands/etc/broker.xml ---
@@ -78,7 +78,7 @@ ${global-max-section}
<!-- amqpLowCredits: The server will send the # credits specified at amqpCredits at this low mark -->
<!-- Acceptor for every supported protocol -->
- <acceptor name="artemis">tcp://${host}:${default.port}?tcpSendBufferSize=1048576;tcpReceiveBufferSize=1048576;protocols=CORE,AMQP,STOMP,HORNETQ,MQTT,OPENWIRE;useEpoll=true;amqpCredits=1000;amqpLowCredits=300</acceptor>
+ <acceptor name="artemis">tcp://${host}:${default.port}?anycastPrefix=jms.queue.;multicastPrefix=jms.topic.;tcpSendBufferSize=1048576;tcpReceiveBufferSize=1048576;protocols=CORE,AMQP,STOMP,HORNETQ,MQTT,OPENWIRE;useEpoll=true;amqpCredits=1000;amqpLowCredits=300</acceptor>
--- End diff --
I didn't want to change default behavior between minor releases therefore I added this config by default.
---
[GitHub] activemq-artemis pull request #1830: ARTEMIS-1644 legacy clients can't acces...
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/1830#discussion_r165080741
--- Diff: artemis-cli/src/main/resources/org/apache/activemq/artemis/cli/commands/etc/broker.xml ---
@@ -78,7 +78,7 @@ ${global-max-section}
<!-- amqpLowCredits: The server will send the # credits specified at amqpCredits at this low mark -->
<!-- Acceptor for every supported protocol -->
- <acceptor name="artemis">tcp://${host}:${default.port}?tcpSendBufferSize=1048576;tcpReceiveBufferSize=1048576;protocols=CORE,AMQP,STOMP,HORNETQ,MQTT,OPENWIRE;useEpoll=true;amqpCredits=1000;amqpLowCredits=300</acceptor>
+ <acceptor name="artemis">tcp://${host}:${default.port}?anycastPrefix=jms.queue.;multicastPrefix=jms.topic.;tcpSendBufferSize=1048576;tcpReceiveBufferSize=1048576;protocols=CORE,AMQP,STOMP,HORNETQ,MQTT,OPENWIRE;useEpoll=true;amqpCredits=1000;amqpLowCredits=300</acceptor>
--- End diff --
@jbertram but isn't this changing the behaviour for old clients? say if you don't change the config.
---
[GitHub] activemq-artemis pull request #1830: ARTEMIS-1644 legacy clients can't acces...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/activemq-artemis/pull/1830
---
[GitHub] activemq-artemis pull request #1830: ARTEMIS-1644 legacy clients can't acces...
Posted by jbertram <gi...@git.apache.org>.
Github user jbertram commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1830#discussion_r165095416
--- Diff: artemis-core-client/src/main/java/org/apache/activemq/artemis/core/protocol/core/impl/wireformat/QueueAbstractPacket.java ---
@@ -69,30 +49,29 @@ public SimpleString getAddress(int clientVersion) {
*/
public final List<SimpleString> convertQueueNames(int clientVersion, List<SimpleString> queueNames) {
if (clientVersion < ADDRESSING_CHANGE_VERSION) {
- return applyAddressPrefixTo(queueNames);
- } else {
- return queueNames;
- }
- }
-
- private List<SimpleString> applyAddressPrefixTo(List<SimpleString> queueNames) {
- final int names = queueNames.size();
- if (names == 0) {
- return Collections.emptyList();
- } else {
- final SimpleString address = this.address;
- final SimpleString prefix = jmsPrefixOf(address);
- if (prefix != null) {
- final List<SimpleString> prefixedQueueNames = new ArrayList<>(names);
- for (int i = 0; i < names; i++) {
- final SimpleString oldQueueNames = queueNames.get(i);
- final SimpleString prefixedQueueName = prefix.concat(oldQueueNames);
- prefixedQueueNames.add(prefixedQueueName);
- }
- return prefixedQueueNames;
+ final int names = queueNames.size();
--- End diff --
> The point of the JIRA is to allow old clients to continue using the old address space. So, I should be able to configure a queue called "jms.queue.foo" and have the old client connect to it. It should not matter what client or protocol you are using.
That's correct, and that use-case should be supported by the PR.
> For this reason, we do not want to strip any prefix in the default case. I.e. there is no prefix set on the acceptor (regardless of the client version).
There is no prefix stripping in this PR. The only thing that the version check is for here is *adding* the prefix for older clients because they still need it even if the acceptor is configured with the prefix. One important change here is to check to see if the prefix is *already* present (e.g. in the case where the address/queue is explicitly configured with it) in which case the prefix won't be added (again).
---
[GitHub] activemq-artemis pull request #1830: ARTEMIS-1644 legacy clients can't acces...
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/1830#discussion_r165076139
--- Diff: artemis-cli/src/main/resources/org/apache/activemq/artemis/cli/commands/etc/broker.xml ---
@@ -78,7 +78,7 @@ ${global-max-section}
<!-- amqpLowCredits: The server will send the # credits specified at amqpCredits at this low mark -->
<!-- Acceptor for every supported protocol -->
- <acceptor name="artemis">tcp://${host}:${default.port}?tcpSendBufferSize=1048576;tcpReceiveBufferSize=1048576;protocols=CORE,AMQP,STOMP,HORNETQ,MQTT,OPENWIRE;useEpoll=true;amqpCredits=1000;amqpLowCredits=300</acceptor>
+ <acceptor name="artemis">tcp://${host}:${default.port}?anycastPrefix=jms.queue.;multicastPrefix=jms.topic.;tcpSendBufferSize=1048576;tcpReceiveBufferSize=1048576;protocols=CORE,AMQP,STOMP,HORNETQ,MQTT,OPENWIRE;useEpoll=true;amqpCredits=1000;amqpLowCredits=300</acceptor>
--- End diff --
shouldn't this be enabled manually by the user when needed? this will add back the old semantics by default. it seems a drawback to me?
---
[GitHub] activemq-artemis pull request #1830: ARTEMIS-1644 legacy clients can't acces...
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/1830#discussion_r165076524
--- Diff: artemis-cli/src/main/resources/org/apache/activemq/artemis/cli/commands/etc/broker.xml ---
@@ -78,7 +78,7 @@ ${global-max-section}
<!-- amqpLowCredits: The server will send the # credits specified at amqpCredits at this low mark -->
<!-- Acceptor for every supported protocol -->
- <acceptor name="artemis">tcp://${host}:${default.port}?tcpSendBufferSize=1048576;tcpReceiveBufferSize=1048576;protocols=CORE,AMQP,STOMP,HORNETQ,MQTT,OPENWIRE;useEpoll=true;amqpCredits=1000;amqpLowCredits=300</acceptor>
+ <acceptor name="artemis">tcp://${host}:${default.port}?anycastPrefix=jms.queue.;multicastPrefix=jms.topic.;tcpSendBufferSize=1048576;tcpReceiveBufferSize=1048576;protocols=CORE,AMQP,STOMP,HORNETQ,MQTT,OPENWIRE;useEpoll=true;amqpCredits=1000;amqpLowCredits=300</acceptor>
--- End diff --
this feels wrong to me.. .@mtaylor
---