-
Notifications
You must be signed in to change notification settings - Fork 916
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ARTEMIS-4455 - Improve message redistribution balance for OFF_WITH_RE… #4644
base: main
Are you sure you want to change the base?
Conversation
that's a semantic change, isn't? OFF_WITH_REDISTRIBUTION should send messages locally , and after a "redistribution-delay" setting messages should then be moved to a different node. The idea is that the local node is always favored. the idea of off_with_redistribution, is that you always route locally, with the possibility of eventually redistributing.... And you have the redistribution delay to respect before you can actually route. This changes goes straight on the routing, making OFF_WITH_REDISTRIBUTION to behave more likely ON_DEMAND. This might work for your usecase, but that semantic would be changed for others who need to always route locally. shouldn't you just use ON_DEMAND and you would get what you need here? You mentioned right here:
And that's the benefit of ON_DEMAND. I'm not sure we should merge this. |
I'm converting this as a draft so we can discuss about it. |
No, there should be no semantic change with regards to initial distribution. Not as far as I've been able to tell at least. Messages still arrive at the local node first, as is expected with OFF_WITH_REDISTRIBUTION. For this LB-type, the matchBinding()-method only returns true for the local binding which then sets the During the "secondary level of routing logic" the local binding is picked as the destination of the message. After that the The bindingIndex in turn, is used by the redistributor (org.apache.activemq.artemis.core.postoffice.impl.BindingsImpl#redistribute()) to set the starting point of it's iteration over the bindings to find a suitable target, meaning that for a continuous stream of incoming messages, the redistributor will pick the same remote Binding as target most of the time, leading to an uneven redistribution of messages. To be clear though, this is my conclusion after poking at this for some time, I might have missed something for sure. |
this change is making at least one test to fail: org.apache.activemq.artemis.tests.integration.amqp.connect.AMQPRedistributeClusterTest.testTopicRedistributionAMQP |
Yes, @clebertsuconic you are right, I missed that one... though looking closer at that test I'm actually getting a bit confused as to what the expected or intended behavior really should be... with ON_DEMAND semantics: First, given topic/multicast semantics for a single node: However, given the same scenario on a two node cluster: Is that the expected behavior? ...I'm also not 100% sure that the test is working as intended, and neither is OFF_WITH_REDISTRIBUTION, this PR or not. Probably the condition: Anyway, In the failing test (org.apache.activemq.artemis.tests.integration.amqp.connect.AMQPRedistributeClusterTest.testTopicRedistributionAMQP), when messages are sent there are no active consumers on any node and as such all messages should end up on just the producers target node (or possibly all messages on all nodes?). This is what happens in the same test when switching over to ON_DEMAND load balancing as well... |
3d02fcd
to
067eaa0
Compare
@clebertsuconic |
…DISTRIBUTION
This benefits the case where messages arrive on a clustered node without a local consumer but with multiple remote targets.
Out of scope for this PR but perhaps it makes sense to implement initial distribution and redistribution weighted on consumerCount of the remote targets so that brokers with more consumers also receive more messages? This could be a nice addition to ON_DEMAND and OFF_WITH_REDISTRIBUTION load balancing types I think.