Skip to content
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-3753 Prevent sending message to internal queues on mirror #4012

Closed
wants to merge 3 commits into from

Conversation

iliya-gr
Copy link
Contributor

@iliya-gr iliya-gr commented Apr 4, 2022

In cluster configuration messages could be routed to internal queues for
further delivering on different broker. We need to check that before
sending to SNF, otherwise message can stuck on target server and will
never receive ACK.

@clebertsuconic
Copy link
Contributor

clebertsuconic commented Apr 5, 2022

who are you? 🥇

Awesome you found this.. nice catch!

Do you have an idea on how to write a test for this?

@iliya-gr
Copy link
Contributor Author

iliya-gr commented Apr 5, 2022

Hi @clebertsuconic

I am just a humble programmer.
I don't see any use from unit testing this, but maybe I will be able to add integration test.
Also there are still some issues with activemq.notifications address, not all messages get ACKed during startup, and I was not able to figure out why.
And I can include https://issues.apache.org/jira/browse/ARTEMIS-3759 to this pull request, that can hide notif queue issue, but won't solve it.

Maybe, just maybe activemq.notifications address also should be marked as internal?

@iliya-gr
Copy link
Contributor Author

iliya-gr commented Apr 7, 2022

I have added integration test for this

@clebertsuconic
Copy link
Contributor

@iliya-gr I will handle this in detail next week.. It got my attention..

@clebertsuconic
Copy link
Contributor

there's an issue with this... what if the queue was sent to an internal queue and a regular queue?

This could happen on a case of a topic distribution in cluster.

although I honestly thought people would be using mirror with Federation instead of clustering, which would offer a better control on the queues and targets.

@clebertsuconic
Copy link
Contributor

clebertsuconic commented Apr 21, 2022

ahhhh... I see... routable would return false only if all the queues are internal queues.

@iliya-gr
Copy link
Contributor Author

although I honestly thought people would be using mirror with Federation instead of clustering, which would offer a better control on the queues and targets.

Yeah and for everyone else it's cool async replication feature. :-)

There is also an issue on the target side witch I can't reproduce locally. One I mentioned earlier: messages from activemq.notifications not getting deleted. It can happen not only with activemq.notifications but with any queue if ACKs follows right after messages and disk is slow. Don't think that source side has something to do with it, just wanted to mention it.

@clebertsuconic
Copy link
Contributor

@iliya-gr are you creating a mirror between two nodes of the same cluster? or that was just for this test?

@iliya-gr
Copy link
Contributor Author

In test I create mirror between one node of a cluster and a regular server. In reality I want to create mirror between each corresponding nodes of two different clusters.

In cluster configuration messages could be routed to internal queues for
further delivering on different broker. We need to check that before
sending to SNF, otherwise message can stuck on target server and will
never receive ACK.
@clebertsuconic
Copy link
Contributor

@iliya-gr can you take a look on #4038...

if you like to rebase against it I may close 4038.

I will think of a test for topics tomorrow... let me think over if I can find of a better solution.

@clebertsuconic
Copy link
Contributor

@iliya-gr I will have to think some more about this.. as we should not route messages to internal queues on the target as well.

I think we should also add a note discouraging clustering message distribution with mirror though.

I will have to think over the weekend... I"m pretty sure the target should route without any internal queues.. otherwise the acks will not send them back to the mirror where they came from.

@iliya-gr
Copy link
Contributor Author

@clebertsuconic I thought it's not possible. Target may send message to internal queue only if it was originally send to internal queue. Maybe just as a precaution of possible acks errors. But maybe I have overlooked something, will check that.

I think we should also add a note discouraging clustering message distribution with mirror though.

I'm not sure I got you right. I think there is nothing wrong in mirror + cluster. I think what is missing is mirror scope/filter parameter that will allow mirroring only specific queues.

@clebertsuconic
Copy link
Contributor

@iliya-gr say you are sending to a destination with strict load balancing... the server on other side of the mirror will then send to its cluster.

@clebertsuconic
Copy link
Contributor

regarding scope/filter.. I thought about adding a filter clause on the mirror. The reason I did not do it was I would need to figure out an efficient way to filter it.. so I postponed it... but if we find a optimal way to filter the addresses we could do it.

@clebertsuconic
Copy link
Contributor

I will resume this on Monday with a fresh mind...

I plan to finish this before we release 2.22

@iliya-gr
Copy link
Contributor Author

@iliya-gr say you are sending to a destination with strict load balancing... the server on other side of the mirror will then send to its cluster.

Got it. I will try to add a test case for this before Monday.

@clebertsuconic
Copy link
Contributor

note for myself (or you) so I won't forget...

  • on the test, The other side of the mirror must have a cluster with Strict load balancing... so messages will be load balanced when sent.
  • on the implementation, I should add a routingType on RoutingContext where I could set the routingType = OFF, and on PostOfficeImpl:;route (probably on Bindings) I would look if the routingType on the context != null, I would ignore any routingType and just the one from routing.
  • The Mirror target will set routingContext.routingType=OFF, so messages will always go to local queues on target.
  • The test should be fixed with these options applied.

@clebertsuconic
Copy link
Contributor

Just to let you know, I found some time to work on it today, and I already have a test locally.. I'm pushing it shortly.

(just to avoid two people working on the same thing)

clebertsuconic pushed a commit to clebertsuconic/activemq-artemis that referenced this pull request Apr 23, 2022
In cluster configuration messages could be routed to internal queues for
further delivering on different broker. We need to check that before
sending to SNF, otherwise message can stuck on target server and will
never receive ACK.

co-author: Clebert Suconic

Discusssions on apache#4012 and apache#4038
@asfgit asfgit closed this in a38fae1 Apr 23, 2022
@clebertsuconic
Copy link
Contributor

merged it...

@clebertsuconic
Copy link
Contributor

I amended everything on a single commit, and I added a mark on myself as a co-author (Hope you don't mind)... just to mention the discussions...

thanks a lot for this!

I am doing a release early next week (look at the dev-list disucssion where I sent a HEADS-UP)

@iliya-gr
Copy link
Contributor Author

Great, thanks for helping me with this!

brusdev pushed a commit to rh-messaging/activemq-artemis that referenced this pull request Apr 27, 2022
In cluster configuration messages could be routed to internal queues for
further delivering on different broker. We need to check that before
sending to SNF, otherwise message can stuck on target server and will
never receive ACK.

co-author: Clebert Suconic

Discusssions on apache/activemq-artemis#4012 and apache/activemq-artemis#4038

(cherry picked from commit 99302b1)

downstream: ENTMQBR-6592
clebertsuconic pushed a commit to clebertsuconic/amq-activemq-artemis that referenced this pull request Jun 29, 2022
In cluster configuration messages could be routed to internal queues for
further delivering on different broker. We need to check that before
sending to SNF, otherwise message can stuck on target server and will
never receive ACK.

co-author: Clebert Suconic

Discusssions on apache/activemq-artemis#4012 and apache/activemq-artemis#4038

(cherry picked from commit 99302b1)
clebertsuconic pushed a commit to clebertsuconic/activemq-artemis that referenced this pull request Jun 29, 2022
In cluster configuration messages could be routed to internal queues for
further delivering on different broker. We need to check that before
sending to SNF, otherwise message can stuck on target server and will
never receive ACK.

co-author: Clebert Suconic

Discusssions on apache#4012 and apache#4038

(cherry picked from commit 99302b1)
clebertsuconic pushed a commit to clebertsuconic/activemq-artemis that referenced this pull request Jun 29, 2022
In cluster configuration messages could be routed to internal queues for
further delivering on different broker. We need to check that before
sending to SNF, otherwise message can stuck on target server and will
never receive ACK.

co-author: Clebert Suconic

Discusssions on apache#4012 and apache#4038

(cherry picked from commit 99302b1)
clebertsuconic pushed a commit to rh-messaging/activemq-artemis that referenced this pull request Jun 29, 2022
In cluster configuration messages could be routed to internal queues for
further delivering on different broker. We need to check that before
sending to SNF, otherwise message can stuck on target server and will
never receive ACK.

co-author: Clebert Suconic

Discusssions on apache/activemq-artemis#4012 and apache/activemq-artemis#4038

(cherry picked from commit 99302b1)

downstream: ENTMQBR-6198
clebertsuconic pushed a commit to clebertsuconic/activemq-artemis that referenced this pull request Aug 18, 2022
In cluster configuration messages could be routed to internal queues for
further delivering on different broker. We need to check that before
sending to SNF, otherwise message can stuck on target server and will
never receive ACK.

co-author: Clebert Suconic

Discusssions on apache#4012 and apache#4038

(cherry picked from commit 99302b1)

downstream: ENTMQBR-6592
clebertsuconic pushed a commit to clebertsuconic/activemq-artemis that referenced this pull request Aug 19, 2022
In cluster configuration messages could be routed to internal queues for
further delivering on different broker. We need to check that before
sending to SNF, otherwise message can stuck on target server and will
never receive ACK.

co-author: Clebert Suconic

Discusssions on apache#4012 and apache#4038

(cherry picked from commit 99302b1)

downstream: ENTMQBR-6592
clebertsuconic pushed a commit to clebertsuconic/activemq-artemis that referenced this pull request Sep 13, 2022
In cluster configuration messages could be routed to internal queues for
further delivering on different broker. We need to check that before
sending to SNF, otherwise message can stuck on target server and will
never receive ACK.

co-author: Clebert Suconic

Discusssions on apache#4012 and apache#4038

(cherry picked from commit 99302b1)
clebertsuconic pushed a commit to clebertsuconic/activemq-artemis that referenced this pull request Sep 14, 2022
In cluster configuration messages could be routed to internal queues for
further delivering on different broker. We need to check that before
sending to SNF, otherwise message can stuck on target server and will
never receive ACK.

co-author: Clebert Suconic

Discusssions on apache#4012 and apache#4038

(cherry picked from commit 99302b1)

downstream: ENTMQBR-6592
clebertsuconic pushed a commit to clebertsuconic/activemq-artemis that referenced this pull request Oct 4, 2022
In cluster configuration messages could be routed to internal queues for
further delivering on different broker. We need to check that before
sending to SNF, otherwise message can stuck on target server and will
never receive ACK.

co-author: Clebert Suconic

Discusssions on apache#4012 and apache#4038

(cherry picked from commit 99302b1)

downstream: ENTMQBR-6592
clebertsuconic pushed a commit to clebertsuconic/activemq-artemis that referenced this pull request Feb 15, 2023
In cluster configuration messages could be routed to internal queues for
further delivering on different broker. We need to check that before
sending to SNF, otherwise message can stuck on target server and will
never receive ACK.

co-author: Clebert Suconic

Discusssions on apache#4012 and apache#4038

(cherry picked from commit 99302b1)

downstream: ENTMQBR-6592
clebertsuconic pushed a commit to clebertsuconic/activemq-artemis that referenced this pull request Feb 17, 2023
In cluster configuration messages could be routed to internal queues for
further delivering on different broker. We need to check that before
sending to SNF, otherwise message can stuck on target server and will
never receive ACK.

co-author: Clebert Suconic

Discusssions on apache#4012 and apache#4038

(cherry picked from commit 99302b1)

downstream: ENTMQBR-6592
clebertsuconic pushed a commit to clebertsuconic/activemq-artemis that referenced this pull request Mar 1, 2023
In cluster configuration messages could be routed to internal queues for
further delivering on different broker. We need to check that before
sending to SNF, otherwise message can stuck on target server and will
never receive ACK.

co-author: Clebert Suconic

Discusssions on apache#4012 and apache#4038

(cherry picked from commit 99302b1)

downstream: ENTMQBR-6592
clebertsuconic pushed a commit to clebertsuconic/activemq-artemis that referenced this pull request Jun 9, 2023
In cluster configuration messages could be routed to internal queues for
further delivering on different broker. We need to check that before
sending to SNF, otherwise message can stuck on target server and will
never receive ACK.

co-author: Clebert Suconic

Discusssions on apache#4012 and apache#4038

(cherry picked from commit 99302b1)

downstream: ENTMQBR-6592
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants