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 2021/11/17 18:39:45 UTC
[GitHub] [activemq-artemis] AntonRoskvist opened a new pull request #3858: ARTEMIS-3557 OFF_WITH_REDISTRIBUTION - Add redistribution to old Anyc…
AntonRoskvist opened a new pull request #3858:
URL: https://github.com/apache/activemq-artemis/pull/3858
…ast consumers
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic edited a comment on pull request #3858: ARTEMIS-3557 OFF_WITH_REDISTRIBUTION - Add redistribution to old Anyc…
Posted by GitBox <gi...@apache.org>.
clebertsuconic edited a comment on pull request #3858:
URL: https://github.com/apache/activemq-artemis/pull/3858#issuecomment-993593587
the commit message here was off.. it should been...
"ARTEMIS-XXX bla bla bla bla...."
I didn't realize it before I merged it.. just for future references
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gtully commented on a change in pull request #3858: ARTEMIS-3557 OFF_WITH_REDISTRIBUTION - Add redistribution to old Anyc…
Posted by GitBox <gi...@apache.org>.
gtully commented on a change in pull request #3858:
URL: https://github.com/apache/activemq-artemis/pull/3858#discussion_r761845128
##########
File path: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/cluster/distribution/ClusterTestBase.java
##########
@@ -751,6 +762,10 @@ protected void sendInRange(final int node,
message.putStringProperty(ClusterTestBase.FILTER_PROP, new SimpleString(filterVal));
}
+ if (routingType != null) {
Review comment:
that may be some special case that requires a sent message to have a routing semantic, it is not normal. Typically a message goes to an address, and the address has a routing type configured across bindings. Ie: it is the broker that determines how a message is routed.
It is non typical to call setRoutingType - it should not be necessary. From a quick peek at git - ARTEMIS-1068 introduced that setter for AMQP, probably for some amqp to jms mapping scenario. But it should not be used or required in this scenario.
I think, have another look.
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gtully commented on pull request #3858: ARTEMIS-3557 OFF_WITH_REDISTRIBUTION - Add redistribution to old Anyc…
Posted by GitBox <gi...@apache.org>.
gtully commented on pull request #3858:
URL: https://github.com/apache/activemq-artemis/pull/3858#issuecomment-985405430
last thing, maybe squash the two commits into one. The auto squash and merge is disabled on this repo :-(
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gtully commented on pull request #3858: ARTEMIS-3557 OFF_WITH_REDISTRIBUTION - Add redistribution to old Anyc…
Posted by GitBox <gi...@apache.org>.
gtully commented on pull request #3858:
URL: https://github.com/apache/activemq-artemis/pull/3858#issuecomment-992703010
I would like some more eyes from folks more familiar with redistribution to be sure to be sure. But this looks good to me.
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gtully commented on pull request #3858: ARTEMIS-3557 OFF_WITH_REDISTRIBUTION - Add redistribution to old Anyc…
Posted by GitBox <gi...@apache.org>.
gtully commented on pull request #3858:
URL: https://github.com/apache/activemq-artemis/pull/3858#issuecomment-979342527
@AntonRoskvist apologies for the delay in response. What was in my mind is the auto create queue case, where the binding is created as part of consumer creation, and on the server that is a two step process, create queue/binding and add consumer. With OFF, any message produced will go to the queue/binding and if the redistributor gets there with a 0 delay, the message may be gone! With the maxHops - potentially never to come back. That window may be very small or maybe there is sufficient sync in there to eliminate it... I don't know for sure. I will need a run in the debugger with a test!
remind me, what is the use case that causes a binding to be created at runtime if not as part of add consumer?
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] AntonRoskvist commented on pull request #3858: ARTEMIS-3557 OFF_WITH_REDISTRIBUTION - Add redistribution to old Anyc…
Posted by GitBox <gi...@apache.org>.
AntonRoskvist commented on pull request #3858:
URL: https://github.com/apache/activemq-artemis/pull/3858#issuecomment-979776866
No problem @gtully .
Well, I'm thinking that if the queue is created by a joining consumer then the broker in question does not have any preexisting messages on that queue (because then the queue would already exist in that case) and since OFF_WITH_REDISTRIBUTION doesn't do any initial distribution the only case I could see where this could happen would be if you create the queue by sending a message to it, and then connect a consumer after... but if you cannot handle messages dispatched to another consumer for the same queue within a cluster then I don't know why you have multiple consumers to begin with?
The binding would also be created if a producer sends a message to a queue that up to that point did not exist on a broker... and probably if just administratively creating a queue, say through the console for instance.
For my case the current behavior becomes an issue if within a cluster there are consumers connected to some brokers but not all and then messages are produced to a broker without a corresponding consumer. In this case no redistribution happens until at least one consumer disconnects and then reconnects. A somewhat similar scenario is outlined here that also should be covered by this change:
https://issues.apache.org/jira/browse/ARTEMIS-3321
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on pull request #3858: ARTEMIS-3557 OFF_WITH_REDISTRIBUTION - Add redistribution to old Anyc…
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on pull request #3858:
URL: https://github.com/apache/activemq-artemis/pull/3858#issuecomment-993593587
the commit message here was off.. it should be
"ARTEMIS-XXX bla bla bla bla...."
I didn't realize it before I merged it.. just for future references
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gtully commented on a change in pull request #3858: ARTEMIS-3557 OFF_WITH_REDISTRIBUTION - Add redistribution to old Anyc…
Posted by GitBox <gi...@apache.org>.
gtully commented on a change in pull request #3858:
URL: https://github.com/apache/activemq-artemis/pull/3858#discussion_r754136911
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/impl/PostOfficeImpl.java
##########
@@ -315,6 +315,37 @@ public void onNotification(final Notification notification) {
queueInfos.put(clusterName, info);
+ if (distance < 1) {
+ //Binding added locally. If a matching remote binding with consumers exist, add a redistributor
+ Binding binding = getBinding(routingName);
+
+ if (binding != null) {
+ try {
+ Bindings bindings = getBindingsForAddress(address);
+
+ for (Binding bind : bindings.getBindings()) {
+ if (bind.isConnected() && bind instanceof RemoteQueueBinding) {
+
+ RemoteQueueBinding remoteBinding = (RemoteQueueBinding) bind;
+
+ if (remoteBinding.consumerCount() > 0) {
+
+ Queue queue = (Queue) binding.getBindable();
+ AddressSettings addressSettings = addressSettingsRepository.getMatch(binding.getAddress().toString());
+ long redistributionDelay = addressSettings.getRedistributionDelay();
+
+ if (redistributionDelay != -1) {
+ queue.addRedistributor(redistributionDelay);
Review comment:
hmm, I think we need a redistribution delay here, if delay is 0 we risk moving messages between binding creation and consumer creation, which is the normal pattern for a JMS consumer.
##########
File path: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/cluster/distribution/ClusterTestBase.java
##########
@@ -723,7 +723,7 @@ protected void sendInRange(final int node,
final int msgEnd,
final boolean durable,
final String filterVal) throws Exception {
- sendInRange(node, address, msgStart, msgEnd, durable, filterVal, null);
Review comment:
these changes don't seem to be necessary for the "redistribute to old bindings" use case?
##########
File path: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/cluster/distribution/MessageRedistributionTest.java
##########
@@ -794,6 +794,46 @@ public void testRedistributionOnlyWhenLocalRemovedLbOffWithRedistribution() thro
verifyReceiveAll(2, 1);
}
+ @Test
+ public void testRedistributionToRemoteConsumerFromNewQueueLbOffWithRedistribution() throws Exception {
+
+ String address = "test.address";
+ String queue = "test.address";
+ String clusterAddress = "test";
+ AddressSettings settings = new AddressSettings().setRedistributionDelay(0).setAutoCreateAddresses(true).setAutoCreateQueues(true);
Review comment:
To have this test work, I needed to add a > 0 setRedistributionDelay, otherwise the assert on 831 - waiting for some message count on 1 would fail b/c the messages were already moved/redistributed.
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/impl/PostOfficeImpl.java
##########
@@ -315,6 +315,37 @@ public void onNotification(final Notification notification) {
queueInfos.put(clusterName, info);
+ if (distance < 1) {
+ //Binding added locally. If a matching remote binding with consumers exist, add a redistributor
+ Binding binding = getBinding(routingName);
+
+ if (binding != null) {
+ try {
+ Bindings bindings = getBindingsForAddress(address);
+
+ for (Binding bind : bindings.getBindings()) {
+ if (bind.isConnected() && bind instanceof RemoteQueueBinding) {
+
+ RemoteQueueBinding remoteBinding = (RemoteQueueBinding) bind;
+
+ if (remoteBinding.consumerCount() > 0) {
+
+ Queue queue = (Queue) binding.getBindable();
+ AddressSettings addressSettings = addressSettingsRepository.getMatch(binding.getAddress().toString());
+ long redistributionDelay = addressSettings.getRedistributionDelay();
+
+ if (redistributionDelay != -1) {
Review comment:
I wonder if we want to break earlier if this condition is not met rather than checking each remoteBinding consumer count etc. maybe check the addressSettings at line:319
##########
File path: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/cluster/distribution/MessageRedistributionTest.java
##########
@@ -794,6 +794,46 @@ public void testRedistributionOnlyWhenLocalRemovedLbOffWithRedistribution() thro
verifyReceiveAll(2, 1);
}
+ @Test
+ public void testRedistributionToRemoteConsumerFromNewQueueLbOffWithRedistribution() throws Exception {
+
+ String address = "test.address";
+ String queue = "test.address";
+ String clusterAddress = "test";
+ AddressSettings settings = new AddressSettings().setRedistributionDelay(0).setAutoCreateAddresses(true).setAutoCreateQueues(true);
+ RoutingType routingType = RoutingType.ANYCAST;
+
+ getServer(0).getAddressSettingsRepository().addMatch(address, settings);
+ getServer(1).getAddressSettingsRepository().addMatch(address, settings);
+
+ setupClusterConnection("cluster0", clusterAddress, MessageLoadBalancingType.OFF_WITH_REDISTRIBUTION, 1, isNetty(), 0, 1);
+ setupClusterConnection("cluster0", clusterAddress, MessageLoadBalancingType.OFF_WITH_REDISTRIBUTION, 1, isNetty(), 1, 0);
+
+ startServers(0, 1);
+
+ setupSessionFactory(0, isNetty());
+ setupSessionFactory(1, isNetty());
+
+ createQueue(0, address, queue, null, true, routingType);
+ addConsumer(0, 0, queue, null);
+ waitForBindings(0, address, 1, 1, true);
+
+ Thread.sleep(3000);
Review comment:
I don't think we need this sleep b/c we are waiting on the binding propagation
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] AntonRoskvist commented on a change in pull request #3858: ARTEMIS-3557 OFF_WITH_REDISTRIBUTION - Add redistribution to old Anyc…
Posted by GitBox <gi...@apache.org>.
AntonRoskvist commented on a change in pull request #3858:
URL: https://github.com/apache/activemq-artemis/pull/3858#discussion_r754331884
##########
File path: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/cluster/distribution/MessageRedistributionTest.java
##########
@@ -794,6 +794,46 @@ public void testRedistributionOnlyWhenLocalRemovedLbOffWithRedistribution() thro
verifyReceiveAll(2, 1);
}
+ @Test
+ public void testRedistributionToRemoteConsumerFromNewQueueLbOffWithRedistribution() throws Exception {
+
+ String address = "test.address";
+ String queue = "test.address";
+ String clusterAddress = "test";
+ AddressSettings settings = new AddressSettings().setRedistributionDelay(0).setAutoCreateAddresses(true).setAutoCreateQueues(true);
+ RoutingType routingType = RoutingType.ANYCAST;
+
+ getServer(0).getAddressSettingsRepository().addMatch(address, settings);
+ getServer(1).getAddressSettingsRepository().addMatch(address, settings);
+
+ setupClusterConnection("cluster0", clusterAddress, MessageLoadBalancingType.OFF_WITH_REDISTRIBUTION, 1, isNetty(), 0, 1);
+ setupClusterConnection("cluster0", clusterAddress, MessageLoadBalancingType.OFF_WITH_REDISTRIBUTION, 1, isNetty(), 1, 0);
+
+ startServers(0, 1);
+
+ setupSessionFactory(0, isNetty());
+ setupSessionFactory(1, isNetty());
+
+ createQueue(0, address, queue, null, true, routingType);
+ addConsumer(0, 0, queue, null);
+ waitForBindings(0, address, 1, 1, true);
+
+ Thread.sleep(3000);
Review comment:
There needs to be some delay here to make sure the notification is received and processed on the other node before creating the same queue there. I will replace it with another "waitForBindings" instead because that's clearly better. Thanks
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] AntonRoskvist commented on a change in pull request #3858: ARTEMIS-3557 OFF_WITH_REDISTRIBUTION - Add redistribution to old Anyc…
Posted by GitBox <gi...@apache.org>.
AntonRoskvist commented on a change in pull request #3858:
URL: https://github.com/apache/activemq-artemis/pull/3858#discussion_r761833005
##########
File path: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/cluster/distribution/ClusterTestBase.java
##########
@@ -751,6 +762,10 @@ protected void sendInRange(final int node,
message.putStringProperty(ClusterTestBase.FILTER_PROP, new SimpleString(filterVal));
}
+ if (routingType != null) {
Review comment:
I think those changes are needed, otherwise send defaults to MULTICAST. Since I am sending messages to a broker without a consumer in the test then those messages would otherwise get discarded... right? I can have another look at it if you think otherwise.
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] AntonRoskvist commented on pull request #3858: ARTEMIS-3557 OFF_WITH_REDISTRIBUTION - Add redistribution to old Anyc…
Posted by GitBox <gi...@apache.org>.
AntonRoskvist commented on pull request #3858:
URL: https://github.com/apache/activemq-artemis/pull/3858#issuecomment-985416535
That test passed in my repo and also when I reran it locally, I figured it was some random issue with Travis.
Should I squash the commits and add it here or close this PR and open a new one? I'm not really sure how to do it against this current PR but perhaps it's simple?
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gtully commented on a change in pull request #3858: ARTEMIS-3557 OFF_WITH_REDISTRIBUTION - Add redistribution to old Anyc…
Posted by GitBox <gi...@apache.org>.
gtully commented on a change in pull request #3858:
URL: https://github.com/apache/activemq-artemis/pull/3858#discussion_r761816877
##########
File path: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/cluster/distribution/ClusterTestBase.java
##########
@@ -751,6 +762,10 @@ protected void sendInRange(final int node,
message.putStringProperty(ClusterTestBase.FILTER_PROP, new SimpleString(filterVal));
}
+ if (routingType != null) {
Review comment:
this bit still seems unnecessary, or am I missing something?
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic merged pull request #3858: ARTEMIS-3557 OFF_WITH_REDISTRIBUTION - Add redistribution to old Anyc…
Posted by GitBox <gi...@apache.org>.
clebertsuconic merged pull request #3858:
URL: https://github.com/apache/activemq-artemis/pull/3858
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] AntonRoskvist commented on a change in pull request #3858: ARTEMIS-3557 OFF_WITH_REDISTRIBUTION - Add redistribution to old Anyc…
Posted by GitBox <gi...@apache.org>.
AntonRoskvist commented on a change in pull request #3858:
URL: https://github.com/apache/activemq-artemis/pull/3858#discussion_r754332372
##########
File path: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/cluster/distribution/ClusterTestBase.java
##########
@@ -723,7 +723,7 @@ protected void sendInRange(final int node,
final int msgEnd,
final boolean durable,
final String filterVal) throws Exception {
- sendInRange(node, address, msgStart, msgEnd, durable, filterVal, null);
Review comment:
You are right, it is not necessary. I will fix it shortly
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gtully commented on pull request #3858: ARTEMIS-3557 OFF_WITH_REDISTRIBUTION - Add redistribution to old Anyc…
Posted by GitBox <gi...@apache.org>.
gtully commented on pull request #3858:
URL: https://github.com/apache/activemq-artemis/pull/3858#issuecomment-985432002
locally squash and force push to this PR brach, or a new PR, whatever works for you. Thanks for your patience.
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] AntonRoskvist commented on pull request #3858: ARTEMIS-3557 OFF_WITH_REDISTRIBUTION - Add redistribution to old Anyc…
Posted by GitBox <gi...@apache.org>.
AntonRoskvist commented on pull request #3858:
URL: https://github.com/apache/activemq-artemis/pull/3858#issuecomment-985438742
@gtully Hope that did it, let me know otherwise
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] AntonRoskvist commented on a change in pull request #3858: ARTEMIS-3557 OFF_WITH_REDISTRIBUTION - Add redistribution to old Anyc…
Posted by GitBox <gi...@apache.org>.
AntonRoskvist commented on a change in pull request #3858:
URL: https://github.com/apache/activemq-artemis/pull/3858#discussion_r754330153
##########
File path: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/cluster/distribution/MessageRedistributionTest.java
##########
@@ -794,6 +794,46 @@ public void testRedistributionOnlyWhenLocalRemovedLbOffWithRedistribution() thro
verifyReceiveAll(2, 1);
}
+ @Test
+ public void testRedistributionToRemoteConsumerFromNewQueueLbOffWithRedistribution() throws Exception {
+
+ String address = "test.address";
+ String queue = "test.address";
+ String clusterAddress = "test";
+ AddressSettings settings = new AddressSettings().setRedistributionDelay(0).setAutoCreateAddresses(true).setAutoCreateQueues(true);
Review comment:
That is very interesting, I have yet to see or even make it happen here... the test always completes with that assert being met. It's not a strictly necessary assert, I added it to make doubly sure there was no initial forwarding happening but that is covered in other tests... the "real" verification is that all messages sent are also received by the "old" consumer. I will have a closer look at that, but otherwise I might just remove that assertion if you don't mind?
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gtully commented on a change in pull request #3858: ARTEMIS-3557 OFF_WITH_REDISTRIBUTION - Add redistribution to old Anyc…
Posted by GitBox <gi...@apache.org>.
gtully commented on a change in pull request #3858:
URL: https://github.com/apache/activemq-artemis/pull/3858#discussion_r761810928
##########
File path: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/cluster/distribution/ClusterTestBase.java
##########
@@ -723,7 +723,7 @@ protected void sendInRange(final int node,
final int msgEnd,
final boolean durable,
final String filterVal) throws Exception {
- sendInRange(node, address, msgStart, msgEnd, durable, filterVal, null);
Review comment:
it looks like the sendInRange change is still present
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gtully commented on pull request #3858: ARTEMIS-3557 OFF_WITH_REDISTRIBUTION - Add redistribution to old Anyc…
Posted by GitBox <gi...@apache.org>.
gtully commented on pull request #3858:
URL: https://github.com/apache/activemq-artemis/pull/3858#issuecomment-985395272
we need to run the full test suite. I peeked at the CI
[ERROR] PagePositionTest>Assert.fail:89 org.apache.activemq.artemis.tests.unit.core.paging.impl.PagePositionTest has left a server socket open on port 61616
that has failed. Does it work ok for you?
I will kick off a rerun. And I need to peek again at your revised changes.
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] AntonRoskvist commented on pull request #3858: ARTEMIS-3557 OFF_WITH_REDISTRIBUTION - Add redistribution to old Anyc…
Posted by GitBox <gi...@apache.org>.
AntonRoskvist commented on pull request #3858:
URL: https://github.com/apache/activemq-artemis/pull/3858#issuecomment-985462088
@gtully Ah, of course. Added a better message now.
I will look at the test but I'm fairly certain it needs to be like this.. I'll get back to you soon about 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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gtully commented on pull request #3858: ARTEMIS-3557 OFF_WITH_REDISTRIBUTION - Add redistribution to old Anyc…
Posted by GitBox <gi...@apache.org>.
gtully commented on pull request #3858:
URL: https://github.com/apache/activemq-artemis/pull/3858#issuecomment-975372999
The new test fails for me, checking for messageCount on server node 1, b/c the redistribution has already happened! Adding a small redistribution delay sorts that, but it is a little flakey.
+++ b/tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/cluster/distribution/MessageRedistributionTest.java
@@ -800,7 +800,7 @@ public class MessageRedistributionTest extends ClusterTestBase {
String address = "test.address";
String queue = "test.address";
String clusterAddress = "test";
- AddressSettings settings = new AddressSettings().setRedistributionDelay(0).setAutoCreateAddresses(true).setAutoCreateQueues(true);
+ AddressSettings settings = new AddressSettings().setRedistributionDelay(100).setAutoCreateAddresses(true).setAutoCreateQueues(true);
Also, it makes me think that adding a redistributor initially will compete in the normal case, where a binding is created before a consumer, if there is no delay, then the consumer may not get any messages. But what should that delay be? Does that need to be configurable? I think it does need to be > 0.
In a typical JMS case, with auto create queue, there is a tiny delay before the consumer is created, so we really must have a delay or the normal expectation will fail.
I guess the point is we are changing/extending the trigger for redistribution, to be something added (a binding) rather than something removed (a consumer).
Maybe in this new added case, we respect the redistribution delay if configured and fallback to 5s if there is none, such that we don't break things.
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] AntonRoskvist commented on a change in pull request #3858: ARTEMIS-3557 OFF_WITH_REDISTRIBUTION - Add redistribution to old Anyc…
Posted by GitBox <gi...@apache.org>.
AntonRoskvist commented on a change in pull request #3858:
URL: https://github.com/apache/activemq-artemis/pull/3858#discussion_r754357267
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/impl/PostOfficeImpl.java
##########
@@ -315,6 +315,37 @@ public void onNotification(final Notification notification) {
queueInfos.put(clusterName, info);
+ if (distance < 1) {
+ //Binding added locally. If a matching remote binding with consumers exist, add a redistributor
+ Binding binding = getBinding(routingName);
+
+ if (binding != null) {
+ try {
+ Bindings bindings = getBindingsForAddress(address);
+
+ for (Binding bind : bindings.getBindings()) {
+ if (bind.isConnected() && bind instanceof RemoteQueueBinding) {
+
+ RemoteQueueBinding remoteBinding = (RemoteQueueBinding) bind;
+
+ if (remoteBinding.consumerCount() > 0) {
+
+ Queue queue = (Queue) binding.getBindable();
+ AddressSettings addressSettings = addressSettingsRepository.getMatch(binding.getAddress().toString());
+ long redistributionDelay = addressSettings.getRedistributionDelay();
+
+ if (redistributionDelay != -1) {
+ queue.addRedistributor(redistributionDelay);
Review comment:
I left a comment below about this, see if you agree, otherwise I will look into adding some form of delay here
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] AntonRoskvist commented on a change in pull request #3858: ARTEMIS-3557 OFF_WITH_REDISTRIBUTION - Add redistribution to old Anyc…
Posted by GitBox <gi...@apache.org>.
AntonRoskvist commented on a change in pull request #3858:
URL: https://github.com/apache/activemq-artemis/pull/3858#discussion_r754358740
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/impl/PostOfficeImpl.java
##########
@@ -315,6 +315,37 @@ public void onNotification(final Notification notification) {
queueInfos.put(clusterName, info);
+ if (distance < 1) {
+ //Binding added locally. If a matching remote binding with consumers exist, add a redistributor
+ Binding binding = getBinding(routingName);
+
+ if (binding != null) {
+ try {
+ Bindings bindings = getBindingsForAddress(address);
+
+ for (Binding bind : bindings.getBindings()) {
+ if (bind.isConnected() && bind instanceof RemoteQueueBinding) {
+
+ RemoteQueueBinding remoteBinding = (RemoteQueueBinding) bind;
+
+ if (remoteBinding.consumerCount() > 0) {
+
+ Queue queue = (Queue) binding.getBindable();
+ AddressSettings addressSettings = addressSettingsRepository.getMatch(binding.getAddress().toString());
+ long redistributionDelay = addressSettings.getRedistributionDelay();
+
+ if (redistributionDelay != -1) {
Review comment:
Yes, that makes a lot of sense, I will look into 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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gtully commented on a change in pull request #3858: ARTEMIS-3557 OFF_WITH_REDISTRIBUTION - Add redistribution to old Anyc…
Posted by GitBox <gi...@apache.org>.
gtully commented on a change in pull request #3858:
URL: https://github.com/apache/activemq-artemis/pull/3858#discussion_r761817130
##########
File path: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/cluster/distribution/ClusterTestBase.java
##########
@@ -751,6 +762,10 @@ protected void sendInRange(final int node,
message.putStringProperty(ClusterTestBase.FILTER_PROP, new SimpleString(filterVal));
}
+ if (routingType != null) {
Review comment:
I don't see the need for any change to the clustertestbase
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] AntonRoskvist commented on pull request #3858: ARTEMIS-3557 OFF_WITH_REDISTRIBUTION - Add redistribution to old Anyc…
Posted by GitBox <gi...@apache.org>.
AntonRoskvist commented on pull request #3858:
URL: https://github.com/apache/activemq-artemis/pull/3858#issuecomment-992272021
I hope this addresses all concerns @gtully , if not just let me know.
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gtully commented on pull request #3858: ARTEMIS-3557 OFF_WITH_REDISTRIBUTION - Add redistribution to old Anyc…
Posted by GitBox <gi...@apache.org>.
gtully commented on pull request #3858:
URL: https://github.com/apache/activemq-artemis/pull/3858#issuecomment-992702048
your test works and a sanity run of the full test suite does not show any regression. I think this is good to go.
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] AntonRoskvist commented on pull request #3858: ARTEMIS-3557 OFF_WITH_REDISTRIBUTION - Add redistribution to old Anyc…
Posted by GitBox <gi...@apache.org>.
AntonRoskvist commented on pull request #3858:
URL: https://github.com/apache/activemq-artemis/pull/3858#issuecomment-985386166
@gtully Is there anything I can do to help getting this PR to go though?
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] AntonRoskvist commented on pull request #3858: ARTEMIS-3557 OFF_WITH_REDISTRIBUTION - Add redistribution to old Anyc…
Posted by GitBox <gi...@apache.org>.
AntonRoskvist commented on pull request #3858:
URL: https://github.com/apache/activemq-artemis/pull/3858#issuecomment-986286264
You were absolutely right about the test @gtully, I mixed it up with the previous Multicast part of this PR that is now broken out... I reverted the testBase completely
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gtully commented on pull request #3858: ARTEMIS-3557 OFF_WITH_REDISTRIBUTION - Add redistribution to old Anyc…
Posted by GitBox <gi...@apache.org>.
gtully commented on pull request #3858:
URL: https://github.com/apache/activemq-artemis/pull/3858#issuecomment-985455206
nearly there, use git --amend to add back in a nice commit message, and again force push.
also, see my comment on the send routing type. thanks!
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] AntonRoskvist commented on pull request #3858: ARTEMIS-3557 OFF_WITH_REDISTRIBUTION - Add redistribution to old Anyc…
Posted by GitBox <gi...@apache.org>.
AntonRoskvist commented on pull request #3858:
URL: https://github.com/apache/activemq-artemis/pull/3858#issuecomment-975611204
@gtully I might be wrong here, but I don't think we need to handle this with additional settings for redistribution delay for that scenario... as it is now a redistributor is added when a remote consumer is added or a local one is removed. The addition here is for a new binding where some remote consumer also exist. In that scenario we would not run into issues with unintentionally moving messages, because since the binding is just now getting created there wont be any messages to redistribute.
Since we are already looking for a redistribution delay before adding the redistributor and the default is "-1" then this will only affect queues that are specifically set up to work with redistribution as well.
I guess there might be a possibility for some messages to get forwarded if the queue is created by a producer and then immediately followed by a consumer... but as soon as the local consumer is added then all redistribution should stop because local consumers are favored anyway. Right?
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org