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.

Sign up for GitHub

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

Jump to bottom

ARTEMIS-4570 filter not applied to all brokers in cluster #4744

Merged
merged 1 commit into from
Jan 23, 2024
Merged

ARTEMIS-4570 filter not applied to all brokers in cluster #4744

merged 1 commit into from
Jan 23, 2024

Conversation

gaohoward
Copy link
Contributor

No description provided.

gtully
gtully reviewed Jan 17, 2024
View reviewed changes
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/QueueImpl.java
@@ -4322,7 +4322,9 @@ public QueueConfiguration getQueueConfiguration() {
.setTemporary(temporary)
.setInternal(internalQueue)
.setTransient(refCountForConsumers instanceof TransientQueueManagerImpl)
.setAutoCreated(autoCreated);
.setAutoCreated(autoCreated)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this change does not seem related, what is going on here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is related to the test, where I get the QueueConfiguration from the queue and use it as the base, change the filter and call PostOffice to updateQueue with it. If I don't add them the test throws exception, like so:
`java.lang.NullPointerException: Cannot invoke "java.lang.Boolean.booleanValue()" because the return value of "org.apache.activemq.artemis.api.core.QueueConfiguration.isGroupRebalancePauseDispatch()" is null

at org.apache.activemq.artemis.core.postoffice.impl.PostOfficeImpl.updateQueue(PostOfficeImpl.java:776)
at org.apache.activemq.artemis.tests.integration.cluster.distribution.MessageLoadBalancingTest.testMessageLoadBalancingWithFiltersUpdate(MessageLoadBalancingTest.java:128)
at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:104)
at java.base/java.lang.reflect.Method.invoke(Method.java:578)
at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:59)
at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:56)
at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:26)
at org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:27)
at org.junit.rules.ExternalResource$1.evaluate(ExternalResource.java:54)
at org.junit.rules.TestWatcher$1.evaluate(TestWatcher.java:61)
at org.junit.rules.TestWatcher$1.evaluate(TestWatcher.java:61)
at org.junit.rules.ExternalResource$1.evaluate(ExternalResource.java:54)
at org.junit.rules.ExternalResource$1.evaluate(ExternalResource.java:54)
at org.junit.rules.TestWatcher$1.evaluate(TestWatcher.java:61)
at org.junit.runners.ParentRunner$3.evaluate(ParentRunner.java:306)
at org.junit.runners.BlockJUnit4ClassRunner$1.evaluate(BlockJUnit4ClassRunner.java:100)
at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:366)
at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:103)
at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:63)
at org.junit.runners.ParentRunner$4.run(ParentRunner.java:331)
at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:79)
at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:329)
at org.junit.runners.ParentRunner.access$100(ParentRunner.java:66)
at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:293)
at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:26)
at org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:27)
at org.junit.rules.TestWatcher$1.evaluate(TestWatcher.java:61)
at org.junit.rules.TestWatcher$1.evaluate(TestWatcher.java:61)
at org.junit.rules.TestWatcher$1.evaluate(TestWatcher.java:61)
at org.junit.rules.TestWatcher$1.evaluate(TestWatcher.java:61)
at org.junit.rules.TestWatcher$1.evaluate(TestWatcher.java:61)
at org.junit.rules.RunRules.evaluate(RunRules.java:20)
at org.junit.runners.ParentRunner$3.evaluate(ParentRunner.java:306)
at org.junit.runners.ParentRunner.run(ParentRunner.java:413)
at org.junit.runner.JUnitCore.run(JUnitCore.java:137)
at com.intellij.junit4.JUnit4IdeaTestRunner.startRunnerWithArgs(JUnit4IdeaTestRunner.java:69)
at com.intellij.rt.junit.IdeaTestRunner$Repeater.startRunnerWithArgs(IdeaTestRunner.java:33)
at com.intellij.rt.junit.JUnitStarter.prepareStreamsAndStart(JUnitStarter.java:235)
at com.intellij.rt.junit.JUnitStarter.main(JUnitStarter.java:54)

`

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm. I don't know whether the update needs to be smarter or every QueueConfiguration must be fully defined. I think the update needs to be smarter b/c there is a semantic meaning to null afaik.

I see another partial QueueConfiguration at

activemq-artemis/artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/impl/PostOfficeImpl.java

Line 695 in 8c41fe6

return updateQueue(new QueueConfiguration(name)

there are a few in PostOfficeImpl. This suggests that the update logic needs to check for the null. I think.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both things are true. The update needs to be smarter to not throw an NPE, but getQueueConfiguration should also be setting all the relevant properties on the QueueConfiguration it returns. In short, I think this is fine and we can follow-up with another PR to deal with the update.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I agree, the update needs to be smarter and also the returned queue config should be complete. I'll do update PR quickly. Thanks guys!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On second thought it would be better to do it in a separate commit/PR as @jbertram said. :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could do it similarly to how I did AddessSettings and that Metadata thingy, on a separate PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@clebertsuconic Hi Clebert can you point me to where you did it? Thanks.

gtully
gtully reviewed Jan 17, 2024
View reviewed changes
Copy link
Contributor

@gtully gtully left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a cluster is expected to be symmetrical, but if for some reason it is not, then there will be an error logged on an unrecognised notification type, which is fine.

@gaohoward
Copy link
Contributor Author

a cluster is expected to be symmetrical, but if for some reason it is not, then there will be an error logged on an unrecognised notification type, which is fine.

yeah that's true.

@gtully
Copy link
Contributor

gtully commented Jan 18, 2024

@clebertsuconic had an interesting observation that changing a filter was previously not allowed, a queue needed to be deleted and recreated with a new filter and those events would be propagated across a cluster. I wonder if there is any potential for having stuck messages with dynamic filters, when the filter becomes more restrictive that would result on unmatched messages that are already on the queue? I guess we could recommend a purge before mod if that is a problem. The bigger question is if a filter should be something that is dynamic?

@gaohoward
Copy link
Contributor Author

I think for topic subscriptions (jms) if you changed the filter the effect is the sub-queue will be recreated (unsubscribed).
if the filter is on the queue side, that's something different.

@clebertsuconic
Copy link
Contributor

Queues are not supposed to be changed... that's something recently added. I don't think your user (I know your user) is actually hitting an update queue.. Did you confirm that?

@gtully
Copy link
Contributor

gtully commented Jan 23, 2024

it can happen from config reload, it seems a valid use case.

@clebertsuconic clebertsuconic merged commit 07b0215 into apache:main Jan 23, 2024
6 checks passed
@gaohoward
Copy link
Contributor Author

yep we should either 1) forbid filter update on both local binding and remote binding, or 1) allow both.
Currently we allow local update but not remote, that make the policy inconsistent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gtully gtully gtully left review comments

@clebertsuconic clebertsuconic clebertsuconic left review comments

@jbertram jbertram jbertram left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
5 participants
@gaohoward @gtully @clebertsuconic @jbertram @howardgao