You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@activemq.apache.org by GitBox <gi...@apache.org> on 2020/01/23 12:27:30 UTC
[GitHub] [activemq-artemis] AntonRoskvist opened a new pull request #2951:
Artemis-2563 Add option to create queue on all active clustered nodes
AntonRoskvist opened a new pull request #2951: Artemis-2563 Add option to create queue on all active clustered nodes
URL: https://github.com/apache/activemq-artemis/pull/2951
This is to help with redistribution as mentioned in this Jira:
https://issues.apache.org/jira/browse/ARTEMIS-2563
Address option added to enable queue creation on all active nodes in a cluster, which happens on BINDING_ADDED. Since this creates a local binding for remote queues, a redistributor will also get created once a consumer registers
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [activemq-artemis] michaelandrepearce commented on a change in pull
request #2951: Artemis-2563 Add option to create queue on all active
clustered nodes
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on a change in pull request #2951: Artemis-2563 Add option to create queue on all active clustered nodes
URL: https://github.com/apache/activemq-artemis/pull/2951#discussion_r371029953
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/settings/impl/AddressSettings.java
##########
@@ -1192,6 +1212,8 @@ public void encode(ActiveMQBuffer buffer) {
BufferHelper.writeNullableBoolean(buffer, defaultLastValueQueue);
BufferHelper.writeNullableLong(buffer, redistributionDelay);
+
+ BufferHelper.writeNullableBoolean(buffer, clusteredQueues);
Review comment:
this cannot be added here, new bits in the journal must ALWAYS be at the end for compatibility reasons
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [activemq-artemis] AntonRoskvist commented on issue #2951:
Artemis-2563 Add option to create queue on all active clustered nodes
Posted by GitBox <gi...@apache.org>.
AntonRoskvist commented on issue #2951: Artemis-2563 Add option to create queue on all active clustered nodes
URL: https://github.com/apache/activemq-artemis/pull/2951#issuecomment-581943348
I will close this for now, got some issues rebasing and formatting. I will open a new one later
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [activemq-artemis] michaelandrepearce commented on a change in pull
request #2951: Artemis-2563 Add option to create queue on all active
clustered nodes
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on a change in pull request #2951: Artemis-2563 Add option to create queue on all active clustered nodes
URL: https://github.com/apache/activemq-artemis/pull/2951#discussion_r371030317
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/impl/PostOfficeImpl.java
##########
@@ -272,6 +272,23 @@ public void onNotification(final Notification notification) {
queueInfos.put(clusterName, info);
+ //Global queue creation fix
+ Binding binding = getBinding(routingName);
+
+ AddressSettings addressSettings = addressSettingsRepository.getMatch(address.toString());
+
+ boolean clusteredQueues = addressSettings.getClusteredQueues();
+
+ if (clusteredQueues && binding == null) {
+ if(routingName.equals(address)) {
+ try {
+ server.createQueue(address, RoutingType.ANYCAST, address, filterString, true, false);
Review comment:
need to support multicast also.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [activemq-artemis] michaelandrepearce commented on a change in pull
request #2951: Artemis-2563 Add option to create queue on all active
clustered nodes
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on a change in pull request #2951: Artemis-2563 Add option to create queue on all active clustered nodes
URL: https://github.com/apache/activemq-artemis/pull/2951#discussion_r371030139
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/impl/PostOfficeImpl.java
##########
@@ -272,6 +272,23 @@ public void onNotification(final Notification notification) {
queueInfos.put(clusterName, info);
+ //Global queue creation fix
+ Binding binding = getBinding(routingName);
+
+ AddressSettings addressSettings = addressSettingsRepository.getMatch(address.toString());
+
+ boolean clusteredQueues = addressSettings.getClusteredQueues();
Review comment:
should this not be a value that's configurable at the queue level, e.g. under an address you may want one queue not clustered and another clustered. it shouldn't be an all or nothing for all queues, e.g. address setting should be a default to apply to queues beneath the address (where the default is false if not set at all), but the actual value should be at the queue level, so that its possible to configure some queues with and some queues not.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [activemq-artemis] michaelandrepearce commented on a change in pull
request #2951: Artemis-2563 Add option to create queue on all active
clustered nodes
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on a change in pull request #2951: Artemis-2563 Add option to create queue on all active clustered nodes
URL: https://github.com/apache/activemq-artemis/pull/2951#discussion_r371030001
##########
File path: artemis-server/src/main/resources/schema/artemis-configuration.xsd
##########
@@ -3276,6 +3276,15 @@
</xsd:documentation>
</xsd:annotation>
</xsd:element>
+
+ <xsd:element name="clustered-queues" type="xsd:boolean" default="false" maxOccurs="1" minOccurs="0">
Review comment:
please add also the the schema in tests, adding tests also around the config.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [activemq-artemis] AntonRoskvist closed pull request #2951:
Artemis-2563 Add option to create queue on all active clustered nodes
Posted by GitBox <gi...@apache.org>.
AntonRoskvist closed pull request #2951: Artemis-2563 Add option to create queue on all active clustered nodes
URL: https://github.com/apache/activemq-artemis/pull/2951
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [activemq-artemis] michaelandrepearce commented on a change in pull
request #2951: Artemis-2563 Add option to create queue on all active
clustered nodes
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on a change in pull request #2951: Artemis-2563 Add option to create queue on all active clustered nodes
URL: https://github.com/apache/activemq-artemis/pull/2951#discussion_r371029975
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/settings/impl/AddressSettings.java
##########
@@ -984,6 +1001,8 @@ public void decode(ActiveMQBuffer buffer, boolean tryCompatible) {
defaultLastValueQueue = BufferHelper.readNullableBoolean(buffer);
redistributionDelay = BufferHelper.readNullableLong(buffer);
+
+ clusteredQueues = BufferHelper.readNullableBoolean(buffer);
Review comment:
must be at the end of the serialisation for compatibility reasons
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [activemq-artemis] clebertsuconic commented on issue #2951:
Artemis-2563 Add option to create queue on all active clustered nodes
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on issue #2951: Artemis-2563 Add option to create queue on all active clustered nodes
URL: https://github.com/apache/activemq-artemis/pull/2951#issuecomment-578219712
- I have not seen any tests added on your PR
- we should have a discussion about the feature itself.. as it may be extremely dangerous. I would prefer having a single subscription on a single node, and clients connecting to that node.
- The PR as it stands it can't be merged.. lack of tests.. you actually are breaking compatibility on the compatility testsuite... (the PR is probably failing because of that).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [activemq-artemis] michaelandrepearce commented on a change in pull
request #2951: Artemis-2563 Add option to create queue on all active
clustered nodes
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on a change in pull request #2951: Artemis-2563 Add option to create queue on all active clustered nodes
URL: https://github.com/apache/activemq-artemis/pull/2951#discussion_r371030139
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/impl/PostOfficeImpl.java
##########
@@ -272,6 +272,23 @@ public void onNotification(final Notification notification) {
queueInfos.put(clusterName, info);
+ //Global queue creation fix
+ Binding binding = getBinding(routingName);
+
+ AddressSettings addressSettings = addressSettingsRepository.getMatch(address.toString());
+
+ boolean clusteredQueues = addressSettings.getClusteredQueues();
Review comment:
should this not be a value that's configurable at the queue level, e.g. under an address you may want one queue not clustered and another clustered. it shouldn't be an all or nothing for all queues, e.g. address setting should be a default, but the actual value should be at the queue level
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [activemq-artemis] AntonRoskvist commented on issue #2951:
Artemis-2563 Add option to create queue on all active clustered nodes
Posted by GitBox <gi...@apache.org>.
AntonRoskvist commented on issue #2951: Artemis-2563 Add option to create queue on all active clustered nodes
URL: https://github.com/apache/activemq-artemis/pull/2951#issuecomment-579225094
Thanks for all of your feedback! I will start working with fixing what you have pointed out. Should I close this PR or can I apply my fixes here somehow?
@clebertsuconic You said
"we should have a discussion about the feature itself.. as it may be extremely dangerous. I would prefer having a single subscription on a single node, and clients connecting to that node."
I do not really understand that last part, how would such an implementation work?
Br,
Anton
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [activemq-artemis] michaelandrepearce commented on a change in pull
request #2951: Artemis-2563 Add option to create queue on all active
clustered nodes
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on a change in pull request #2951: Artemis-2563 Add option to create queue on all active clustered nodes
URL: https://github.com/apache/activemq-artemis/pull/2951#discussion_r371030260
##########
File path: docs/user-manual/en/address-model.md
##########
@@ -720,6 +721,12 @@ before it will begin to dispatch messages. Default is `-1` (wait forever).
closed on a queue before redistributing any messages. Read more about
[clusters](clusters.md#message-redistribution).
+`clustered-queues` enables cluster-wide creation of `ANYCAST` queues.
Review comment:
why only anycast, surely queue type should not matter, and use cases for mutlicast would exist, e.g. share durable subscriptions
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services