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/06/09 15:58:47 UTC
[GitHub] [activemq-artemis] jsmucr opened a new pull request #3172: ARTEMIS-2797 - Allow for removing current queue filter
jsmucr opened a new pull request #3172:
URL: https://github.com/apache/activemq-artemis/pull/3172
Now it is possible to remove queue filter by setting it to an empty string or null.
Before this commit the server would just ignore such request.
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] michaelandrepearce commented on a change in pull request #3172: ARTEMIS-2797 - Allow for removing current queue filter
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on a change in pull request #3172:
URL: https://github.com/apache/activemq-artemis/pull/3172#discussion_r437906371
##########
File path: tests/integration-tests/src/test/resources/reload-queue-filter-updated-empty.xml
##########
@@ -0,0 +1,43 @@
+<?xml version='1.0'?>
+<!--
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements. See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership. The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing,
+software distributed under the License is distributed on an
+"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+KIND, either express or implied. See the License for the
+specific language governing permissions and limitations
+under the License.
+-->
+
+<configuration xmlns="urn:activemq"
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xsi:schemaLocation="urn:activemq /schema/artemis-configuration.xsd">
+
+ <core xmlns="urn:activemq:core">
+ <security-enabled>false</security-enabled>
+ <persistence-enabled>false</persistence-enabled>
+
+ <acceptors>
+ <acceptor name="artemis">tcp://0.0.0.0:61616</acceptor>
+ </acceptors>
+
+ <addresses>
+ <address name="myFilterQueue">
+ <anycast>
+ <queue name="myFilterQueue">
+ <filter string=""/>
Review comment:
This isnt filtering. I would expect the test to be a real filter string not an empty one. Empty filter is same as no filter so this isnt validating much
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] michaelpearce-gain commented on pull request #3172: ARTEMIS-2797 - Reset queue properties by unsetting them in broker.xml
Posted by GitBox <gi...@apache.org>.
michaelpearce-gain commented on pull request #3172:
URL: https://github.com/apache/activemq-artemis/pull/3172#issuecomment-648002327
@jsmucr can you rebase and squash commits
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] michaelpearce-gain commented on a change in pull request #3172: ARTEMIS-2797 - Allow for removing current queue filter
Posted by GitBox <gi...@apache.org>.
michaelpearce-gain commented on a change in pull request #3172:
URL: https://github.com/apache/activemq-artemis/pull/3172#discussion_r438255463
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/impl/PostOfficeImpl.java
##########
@@ -689,7 +689,10 @@ public QueueBinding updateQueue(QueueConfiguration queueConfiguration) throws Ex
queue.setDelayBeforeDispatch(queueConfiguration.getDelayBeforeDispatch().longValue());
}
Filter filter = FilterImpl.createFilter(queueConfiguration.getFilterString());
- if (filter != null && !filter.equals(queue.getFilter())) {
+ if ((filter == null) && (queue.getFilter() != null)) {
Review comment:
I can actually see same issue with trying to clear groupFirstKey, e.g. you can't clear it, so i understand your issue, just trying to think how to achieve this unilaterally and safely. Def needs docs what ever we do.
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] michaelpearce-gain commented on a change in pull request #3172: ARTEMIS-2797 - Allow for removing current queue filter
Posted by GitBox <gi...@apache.org>.
michaelpearce-gain commented on a change in pull request #3172:
URL: https://github.com/apache/activemq-artemis/pull/3172#discussion_r438621261
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/impl/PostOfficeImpl.java
##########
@@ -647,66 +649,121 @@ public QueueBinding updateQueue(QueueConfiguration queueConfiguration) throws Ex
}
}
- //atomic update
- if (queueConfiguration.getMaxConsumers() != null && queue.getMaxConsumers() != queueConfiguration.getMaxConsumers().intValue()) {
+ // atomic update, reset to defaults if value == null
+ // maxConsumers
+ final int newMaxConsumers = queueConfiguration.getMaxConsumers() == null
+ ? ActiveMQDefaultConfiguration.getDefaultMaxQueueConsumers()
+ : queueConfiguration.getMaxConsumers();
+ if (queue.getMaxConsumers() != newMaxConsumers) {
changed = true;
- queue.setMaxConsumer(queueConfiguration.getMaxConsumers());
+ queue.setMaxConsumer(newMaxConsumers);
}
- if (queueConfiguration.getRoutingType() != null && queue.getRoutingType() != queueConfiguration.getRoutingType()) {
+ // routingType
+ final RoutingType newRoutingType = queueConfiguration.getRoutingType() == null
Review comment:
e.g. you could just do this once before the section of atomic updates.
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] jsmucr commented on a change in pull request #3172: ARTEMIS-2797 - Allow for removing current queue filter
Posted by GitBox <gi...@apache.org>.
jsmucr commented on a change in pull request #3172:
URL: https://github.com/apache/activemq-artemis/pull/3172#discussion_r437973130
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/impl/PostOfficeImpl.java
##########
@@ -689,7 +689,10 @@ public QueueBinding updateQueue(QueueConfiguration queueConfiguration) throws Ex
queue.setDelayBeforeDispatch(queueConfiguration.getDelayBeforeDispatch().longValue());
}
Filter filter = FilterImpl.createFilter(queueConfiguration.getFilterString());
- if (filter != null && !filter.equals(queue.getFilter())) {
+ if ((filter == null) && (queue.getFilter() != null)) {
Review comment:
Hm, I must have overlooked the fact that the other properties behave the same too.
Is the behavior I propose a correct one? Or is there a reason for ignoring null values?
Now that it's obvious - even to me :-) - that the issue is a lot broader, shall I continue with implementing the rest here or would that be a whole new pull request?
My use case in an automatic cluster configuration deployment and the current behavior sort of breaks the idea.
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] michaelandrepearce commented on pull request #3172: ARTEMIS-2797 - Allow for removing current queue filter
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on pull request #3172:
URL: https://github.com/apache/activemq-artemis/pull/3172#issuecomment-645229051
Looking alot better. I note you still have this as draft pr. Is there more outstanding?
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] michaelpearce-gain commented on a change in pull request #3172: ARTEMIS-2797 - Allow for removing current queue filter
Posted by GitBox <gi...@apache.org>.
michaelpearce-gain commented on a change in pull request #3172:
URL: https://github.com/apache/activemq-artemis/pull/3172#discussion_r438620908
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/impl/PostOfficeImpl.java
##########
@@ -647,66 +649,121 @@ public QueueBinding updateQueue(QueueConfiguration queueConfiguration) throws Ex
}
}
- //atomic update
- if (queueConfiguration.getMaxConsumers() != null && queue.getMaxConsumers() != queueConfiguration.getMaxConsumers().intValue()) {
+ // atomic update, reset to defaults if value == null
+ // maxConsumers
+ final int newMaxConsumers = queueConfiguration.getMaxConsumers() == null
+ ? ActiveMQDefaultConfiguration.getDefaultMaxQueueConsumers()
+ : queueConfiguration.getMaxConsumers();
+ if (queue.getMaxConsumers() != newMaxConsumers) {
changed = true;
- queue.setMaxConsumer(queueConfiguration.getMaxConsumers());
+ queue.setMaxConsumer(newMaxConsumers);
}
- if (queueConfiguration.getRoutingType() != null && queue.getRoutingType() != queueConfiguration.getRoutingType()) {
+ // routingType
+ final RoutingType newRoutingType = queueConfiguration.getRoutingType() == null
Review comment:
So to get defaults, should be using the address settings.
Theres actually a common method that does this thats used else where, as its quite common
QueueConfigurationUtils.applyDynamicQueueDefaults(queueConfiguration, addressSettingsRepository.getMatch(queueConfiguration.getAddress().toString()));
This then will correctly set defaults on everything in the queueConfiguration
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] jsmucr commented on a change in pull request #3172: ARTEMIS-2797 - Allow for removing current queue filter
Posted by GitBox <gi...@apache.org>.
jsmucr commented on a change in pull request #3172:
URL: https://github.com/apache/activemq-artemis/pull/3172#discussion_r438543888
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/impl/PostOfficeImpl.java
##########
@@ -689,7 +689,10 @@ public QueueBinding updateQueue(QueueConfiguration queueConfiguration) throws Ex
queue.setDelayBeforeDispatch(queueConfiguration.getDelayBeforeDispatch().longValue());
}
Filter filter = FilterImpl.createFilter(queueConfiguration.getFilterString());
- if (filter != null && !filter.equals(queue.getFilter())) {
+ if ((filter == null) && (queue.getFilter() != null)) {
Review comment:
Clearing the value using `""` does not work, at least not for a filter.
```
//...
public static Filter createFilter(final SimpleString filterStr) throws ActiveMQException {
if (filterStr == null || filterStr.length() == 0) {
return null;
}
// ...
```
So there would have to be a breaking change anyway, just to achieve this.
I understand the issue with resetting the whole configuration. Maybe there could be a toggle to enable this.
I'm working for a cloud-based EDI provider and our ultimate goal is to have all of our infrastructure configurable via Ansible (or similar tools) so that in case of a major failure there'd be no need to perform too many manual steps in order to rebuild the service from scratch. That makes the Ansible repository the only place to store our configuration, and therefore it would be highly beneficial if I could redeploy the Artemis configuration from there. :-) It would also open the way to create a bit more user friendly tool to design the MQ related infrastructure.
I'd be happy to hear about the way you do it in your company. :-)
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] michaelpearce-gain edited a comment on pull request #3172: ARTEMIS-2797 - Reset queue properties by unsetting them in broker.xml
Posted by GitBox <gi...@apache.org>.
michaelpearce-gain edited a comment on pull request #3172:
URL: https://github.com/apache/activemq-artemis/pull/3172#issuecomment-647986368
@jsmucr LGTM, if you get docs done, then we can merge i think.
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] michaelpearce-gain commented on pull request #3172: ARTEMIS-2797 - Reset queue properties by unsetting them in broker.xml
Posted by GitBox <gi...@apache.org>.
michaelpearce-gain commented on pull request #3172:
URL: https://github.com/apache/activemq-artemis/pull/3172#issuecomment-647986368
@jsmucr LGTM
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] michaelpearce-gain commented on a change in pull request #3172: ARTEMIS-2797 - Allow for removing current queue filter
Posted by GitBox <gi...@apache.org>.
michaelpearce-gain commented on a change in pull request #3172:
URL: https://github.com/apache/activemq-artemis/pull/3172#discussion_r438123313
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/impl/PostOfficeImpl.java
##########
@@ -689,7 +689,10 @@ public QueueBinding updateQueue(QueueConfiguration queueConfiguration) throws Ex
queue.setDelayBeforeDispatch(queueConfiguration.getDelayBeforeDispatch().longValue());
}
Filter filter = FilterImpl.createFilter(queueConfiguration.getFilterString());
- if (filter != null && !filter.equals(queue.getFilter())) {
+ if ((filter == null) && (queue.getFilter() != null)) {
Review comment:
So question here is what is the challenge you have right now? Btw i dont work for a service provider but actually an arch at a financial firm that uses artemis (been using for many yrs), and we manage alot of the broker configuration automatically so be interesting what challenge you have which we havent had to solve.
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] michaelpearce-gain commented on a change in pull request #3172: ARTEMIS-2797 - Allow for removing current queue filter
Posted by GitBox <gi...@apache.org>.
michaelpearce-gain commented on a change in pull request #3172:
URL: https://github.com/apache/activemq-artemis/pull/3172#discussion_r438123727
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/impl/PostOfficeImpl.java
##########
@@ -689,7 +689,10 @@ public QueueBinding updateQueue(QueueConfiguration queueConfiguration) throws Ex
queue.setDelayBeforeDispatch(queueConfiguration.getDelayBeforeDispatch().longValue());
}
Filter filter = FilterImpl.createFilter(queueConfiguration.getFilterString());
- if (filter != null && !filter.equals(queue.getFilter())) {
+ if ((filter == null) && (queue.getFilter() != null)) {
Review comment:
The idea of the null checks, is so that you update just a partial part, e,g, if i only wanted to update say exclusive consumer, i wouldnt have to know all the rest of the config. And there for the update had to be a positive (e.g. it was set) change, to avoid an accident. Maybe could be that the approach here is to use "" empty string as a single its a positive change, e.g. theres a difference semantically in that its been set to "" its not null, and there for then could use that to clear it.
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] jsmucr commented on a change in pull request #3172: ARTEMIS-2797 - Allow for removing current queue filter
Posted by GitBox <gi...@apache.org>.
jsmucr commented on a change in pull request #3172:
URL: https://github.com/apache/activemq-artemis/pull/3172#discussion_r437959670
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/impl/PostOfficeImpl.java
##########
@@ -689,7 +689,10 @@ public QueueBinding updateQueue(QueueConfiguration queueConfiguration) throws Ex
queue.setDelayBeforeDispatch(queueConfiguration.getDelayBeforeDispatch().longValue());
}
Filter filter = FilterImpl.createFilter(queueConfiguration.getFilterString());
- if (filter != null && !filter.equals(queue.getFilter())) {
+ if ((filter == null) && (queue.getFilter() != null)) {
Review comment:
I see. I'll take look at 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
[GitHub] [activemq-artemis] jsmucr commented on a change in pull request #3172: ARTEMIS-2797 - Allow for removing current queue filter
Posted by GitBox <gi...@apache.org>.
jsmucr commented on a change in pull request #3172:
URL: https://github.com/apache/activemq-artemis/pull/3172#discussion_r437892289
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/impl/PostOfficeImpl.java
##########
@@ -689,7 +689,10 @@ public QueueBinding updateQueue(QueueConfiguration queueConfiguration) throws Ex
queue.setDelayBeforeDispatch(queueConfiguration.getDelayBeforeDispatch().longValue());
}
Filter filter = FilterImpl.createFilter(queueConfiguration.getFilterString());
- if (filter != null && !filter.equals(queue.getFilter())) {
+ if ((filter == null) && (queue.getFilter() != null)) {
Review comment:
I've added the config reload test.
I'm not sure about what you mean by the default behavior. Shall I add some sort of constant (like `DEFAULT_FILTER`) or just document the fact that there's no filter by default and that setting it to `null` (by removing it from the configuration) or to `""` reverts it back to the default value?
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] michaelpearce-gain commented on a change in pull request #3172: ARTEMIS-2797 - Allow for removing current queue filter
Posted by GitBox <gi...@apache.org>.
michaelpearce-gain commented on a change in pull request #3172:
URL: https://github.com/apache/activemq-artemis/pull/3172#discussion_r438123727
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/impl/PostOfficeImpl.java
##########
@@ -689,7 +689,10 @@ public QueueBinding updateQueue(QueueConfiguration queueConfiguration) throws Ex
queue.setDelayBeforeDispatch(queueConfiguration.getDelayBeforeDispatch().longValue());
}
Filter filter = FilterImpl.createFilter(queueConfiguration.getFilterString());
- if (filter != null && !filter.equals(queue.getFilter())) {
+ if ((filter == null) && (queue.getFilter() != null)) {
Review comment:
The idea of the null checks, is so that you update just a partial part, e,g, if i only wanted to update say exclusive consumer, i wouldnt have to know all the rest of the config. And there for the update had to be a positive change.
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] michaelandrepearce commented on a change in pull request #3172: ARTEMIS-2797 - Allow for removing current queue filter
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on a change in pull request #3172:
URL: https://github.com/apache/activemq-artemis/pull/3172#discussion_r437908090
##########
File path: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/jms/RedeployTest.java
##########
@@ -287,6 +287,99 @@ public void run() {
}
}
+ private void doTestRemoveFilter(URL testConfiguration) throws Exception {
+
+ Path brokerXML = getTestDirfile().toPath().resolve("broker.xml");
+
+ URL baseConfig = RedeployTest.class.getClassLoader().getResource("reload-queue-filter.xml");
+
+ Files.copy(baseConfig.openStream(), brokerXML, StandardCopyOption.REPLACE_EXISTING);
+
+ EmbeddedActiveMQ embeddedActiveMQ = new EmbeddedActiveMQ();
+ embeddedActiveMQ.setConfigResourcePath(brokerXML.toUri().toString());
+ embeddedActiveMQ.start();
+
+ final ReusableLatch latch = new ReusableLatch(1);
+
+ Runnable tick = new Runnable() {
+ @Override
+ public void run() {
+ latch.countDown();
+ }
+ };
+
+ try {
+
+ embeddedActiveMQ.getActiveMQServer().getReloadManager().setTick(tick);
+ latch.await(10, TimeUnit.SECONDS);
+
+ try (ActiveMQConnectionFactory factory = new ActiveMQConnectionFactory();
+ Connection connection = factory.createConnection();
+ Session session = connection.createSession(Session.AUTO_ACKNOWLEDGE)) {
+
+ connection.start();
+ Queue queue = session.createQueue("myFilterQueue");
+ MessageProducer producer = session.createProducer(queue);
+
+ Message passingMessage = session.createMessage();
+ passingMessage.setStringProperty("x", "x");
+ producer.send(passingMessage);
+
+ Message filteredMessage = session.createMessage();
+ filteredMessage.setStringProperty("x", "y");
+ producer.send(filteredMessage);
+
+ MessageConsumer consumer = session.createConsumer(queue);
+ Message receivedMessage = consumer.receive(2000);
+ assertNotNull(receivedMessage);
+ assertEquals("x", receivedMessage.getStringProperty("x"));
+
+ assertNull(consumer.receive(2000));
+
+ consumer.close();
+ }
+
+ Files.copy(testConfiguration.openStream(), brokerXML, StandardCopyOption.REPLACE_EXISTING);
+ brokerXML.toFile().setLastModified(System.currentTimeMillis() + 1000);
+
+ latch.setCount(1);
+ embeddedActiveMQ.getActiveMQServer().getReloadManager().setTick(tick);
+ latch.await(10, TimeUnit.SECONDS);
+
+ try (ActiveMQConnectionFactory factory = new ActiveMQConnectionFactory();
+ Connection connection = factory.createConnection();
+ Session session = connection.createSession(Session.AUTO_ACKNOWLEDGE)) {
+
+ connection.start();
+ Queue queue = session.createQueue("myFilterQueue");
+ MessageProducer producer = session.createProducer(queue);
+
+ Message message1 = session.createMessage();
+ message1.setStringProperty("x", "x");
+ producer.send(message1);
+
+ Message message2 = session.createMessage();
+ message2.setStringProperty("x", "y");
+ producer.send(message2);
+
+ MessageConsumer consumer = session.createConsumer(queue);
+ assertNotNull(consumer.receive(2000));
+ assertNotNull(consumer.receive(2000));
+
+ consumer.close();
+ }
+
+ } finally {
+ embeddedActiveMQ.stop();
+ }
+ }
+
+ @Test
+ public void testRedeployRemoveFilter() throws Exception {
+ doTestRemoveFilter(RedeployTest.class.getClassLoader().getResource("reload-queue-filter-updated-empty.xml"));
+ doTestRemoveFilter(RedeployTest.class.getClassLoader().getResource("reload-queue-filter-removed.xml"));
+ }
Review comment:
I would expect the test to first validate a filter works and queueimpl has filter matching with a real filter, then reload config and validate both the queueimpl is null and now no filtering is applied on flow. To vaildate update does indeed remove a filter if previously set.
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] michaelpearce-gain commented on a change in pull request #3172: ARTEMIS-2797 - Allow for removing current queue filter
Posted by GitBox <gi...@apache.org>.
michaelpearce-gain commented on a change in pull request #3172:
URL: https://github.com/apache/activemq-artemis/pull/3172#discussion_r438123727
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/impl/PostOfficeImpl.java
##########
@@ -689,7 +689,10 @@ public QueueBinding updateQueue(QueueConfiguration queueConfiguration) throws Ex
queue.setDelayBeforeDispatch(queueConfiguration.getDelayBeforeDispatch().longValue());
}
Filter filter = FilterImpl.createFilter(queueConfiguration.getFilterString());
- if (filter != null && !filter.equals(queue.getFilter())) {
+ if ((filter == null) && (queue.getFilter() != null)) {
Review comment:
The idea of the null checks, is so that you update just a partial part, e,g, if i only wanted to update say exclusive consumer, i wouldnt have to know all the rest of the config. And there for the update had to be a positive (e.g. it was set) change.
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] michaelandrepearce commented on a change in pull request #3172: ARTEMIS-2797 - Allow for removing current queue filter
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on a change in pull request #3172:
URL: https://github.com/apache/activemq-artemis/pull/3172#discussion_r437564056
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/impl/PostOfficeImpl.java
##########
@@ -689,7 +689,10 @@ public QueueBinding updateQueue(QueueConfiguration queueConfiguration) throws Ex
queue.setDelayBeforeDispatch(queueConfiguration.getDelayBeforeDispatch().longValue());
}
Filter filter = FilterImpl.createFilter(queueConfiguration.getFilterString());
- if (filter != null && !filter.equals(queue.getFilter())) {
+ if ((filter == null) && (queue.getFilter() != null)) {
Review comment:
Should think about applying the same to all, e.g. if now null set back to default for address setting (or static default)
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/impl/PostOfficeImpl.java
##########
@@ -689,7 +689,10 @@ public QueueBinding updateQueue(QueueConfiguration queueConfiguration) throws Ex
queue.setDelayBeforeDispatch(queueConfiguration.getDelayBeforeDispatch().longValue());
}
Filter filter = FilterImpl.createFilter(queueConfiguration.getFilterString());
- if (filter != null && !filter.equals(queue.getFilter())) {
+ if ((filter == null) && (queue.getFilter() != null)) {
Review comment:
Should think about applying the same to all, e.g. if now null set back to default for address setting (or static default) else some users will get different behaviour of what update does per queue config. We should have uniform behaviour across all. E.g. if support here should have similar logic ob all. And also def requires doc updates
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/impl/PostOfficeImpl.java
##########
@@ -689,7 +689,10 @@ public QueueBinding updateQueue(QueueConfiguration queueConfiguration) throws Ex
queue.setDelayBeforeDispatch(queueConfiguration.getDelayBeforeDispatch().longValue());
}
Filter filter = FilterImpl.createFilter(queueConfiguration.getFilterString());
- if (filter != null && !filter.equals(queue.getFilter())) {
+ if ((filter == null) && (queue.getFilter() != null)) {
Review comment:
Should think about applying the same to all, e.g. if now null set back to default for address setting (or static default) else some users will get different behaviour of what update does per queue config. We should have uniform behaviour across all. E.g. if support here should have similar logic ob all. And also def requires doc updates, and a call out in behaviour changes in release notes
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] michaelpearce-gain edited a comment on pull request #3172: ARTEMIS-2797 - Reset queue properties by unsetting them in broker.xml
Posted by GitBox <gi...@apache.org>.
michaelpearce-gain edited a comment on pull request #3172:
URL: https://github.com/apache/activemq-artemis/pull/3172#issuecomment-647986368
@jsmucr LGTM, if you get docs done, then we can merge i think. Not sure if you're following the mail thread but @clebertsuconic is aiming to cut a release today/tomorrow so if you get it done in time hopefully it can make it in for you.
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] michaelandrepearce commented on a change in pull request #3172: ARTEMIS-2797 - Allow for removing current queue filter
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on a change in pull request #3172:
URL: https://github.com/apache/activemq-artemis/pull/3172#discussion_r437904731
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/impl/PostOfficeImpl.java
##########
@@ -689,7 +689,10 @@ public QueueBinding updateQueue(QueueConfiguration queueConfiguration) throws Ex
queue.setDelayBeforeDispatch(queueConfiguration.getDelayBeforeDispatch().longValue());
}
Filter filter = FilterImpl.createFilter(queueConfiguration.getFilterString());
- if (filter != null && !filter.equals(queue.getFilter())) {
+ if ((filter == null) && (queue.getFilter() != null)) {
Review comment:
I was meaning other queue configurations that you can update e.g like the one immediately above delaybeforedispatch. E.g. if change of behaviour from currently if null do not update, now youre proposing to make it if null set back to default which for filter is null. We would want that behaviour consistent. Else we end up with some ignoring and some reverting.
Its very important behaviour is consistent for all, so either all do nothing when new value is null (ignore) current behaviour before your proposed change or all update back to their defaults (being either the default at address setting or where a default constant its used)
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] michaelandrepearce commented on a change in pull request #3172: ARTEMIS-2797 - Allow for removing current queue filter
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on a change in pull request #3172:
URL: https://github.com/apache/activemq-artemis/pull/3172#discussion_r437904731
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/impl/PostOfficeImpl.java
##########
@@ -689,7 +689,10 @@ public QueueBinding updateQueue(QueueConfiguration queueConfiguration) throws Ex
queue.setDelayBeforeDispatch(queueConfiguration.getDelayBeforeDispatch().longValue());
}
Filter filter = FilterImpl.createFilter(queueConfiguration.getFilterString());
- if (filter != null && !filter.equals(queue.getFilter())) {
+ if ((filter == null) && (queue.getFilter() != null)) {
Review comment:
I was meaning other queue configurations that you can update e.g like the one immediately above delaybeforedispatch. E.g. if change of behaviour from currently if null do not update, now youre proposing to make it if null set back to default which for filter is null. We would want that behaviour consistent. Else we end up with some ignoring and some reverting.
Its very important behaviour is consistent for all, so either all do nothing when new value is null (ignore) or all update back to their defaults (being either the default at address setting or where a default constant its used)
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] michaelpearce-gain commented on a change in pull request #3172: ARTEMIS-2797 - Allow for removing current queue filter
Posted by GitBox <gi...@apache.org>.
michaelpearce-gain commented on a change in pull request #3172:
URL: https://github.com/apache/activemq-artemis/pull/3172#discussion_r438253085
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/impl/PostOfficeImpl.java
##########
@@ -689,7 +689,10 @@ public QueueBinding updateQueue(QueueConfiguration queueConfiguration) throws Ex
queue.setDelayBeforeDispatch(queueConfiguration.getDelayBeforeDispatch().longValue());
}
Filter filter = FilterImpl.createFilter(queueConfiguration.getFilterString());
- if (filter != null && !filter.equals(queue.getFilter())) {
+ if ((filter == null) && (queue.getFilter() != null)) {
Review comment:
Looking further, you could probably safely apply the null thing on all of them, if you defaulted values (the same was as one on create) using maybe somewhere around line 633 (essentially before the checks)
QueueConfigurationUtils.applyDynamicQueueDefaults(queueConfiguration, addressSettingsRepository.getMatch(queueConfiguration.getAddress().toString()));
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] jsmucr commented on pull request #3172: ARTEMIS-2797 - Allow for removing current queue filter
Posted by GitBox <gi...@apache.org>.
jsmucr commented on pull request #3172:
URL: https://github.com/apache/activemq-artemis/pull/3172#issuecomment-647922106
Waiting for someone to check this before I continue with docs.
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] michaelpearce-gain commented on pull request #3172: ARTEMIS-2797 - Reset queue properties by unsetting them in broker.xml
Posted by GitBox <gi...@apache.org>.
michaelpearce-gain commented on pull request #3172:
URL: https://github.com/apache/activemq-artemis/pull/3172#issuecomment-648050524
@jsmucr thanks for the contribution.
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] jsmucr commented on a change in pull request #3172: ARTEMIS-2797 - Allow for removing current queue filter
Posted by GitBox <gi...@apache.org>.
jsmucr commented on a change in pull request #3172:
URL: https://github.com/apache/activemq-artemis/pull/3172#discussion_r437935836
##########
File path: tests/integration-tests/src/test/resources/reload-queue-filter-updated-empty.xml
##########
@@ -0,0 +1,43 @@
+<?xml version='1.0'?>
+<!--
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements. See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership. The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing,
+software distributed under the License is distributed on an
+"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+KIND, either express or implied. See the License for the
+specific language governing permissions and limitations
+under the License.
+-->
+
+<configuration xmlns="urn:activemq"
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xsi:schemaLocation="urn:activemq /schema/artemis-configuration.xsd">
+
+ <core xmlns="urn:activemq:core">
+ <security-enabled>false</security-enabled>
+ <persistence-enabled>false</persistence-enabled>
+
+ <acceptors>
+ <acceptor name="artemis">tcp://0.0.0.0:61616</acceptor>
+ </acceptors>
+
+ <addresses>
+ <address name="myFilterQueue">
+ <anycast>
+ <queue name="myFilterQueue">
+ <filter string=""/>
Review comment:
The `doTestRemoveFilter` method first applies the `reload-queue-filter.xml` configuration that has the filter set up correctly, then tests if it works and after that it applies the intended modification (either `reload-queue-filter-updated-empty.xml` or `reload-queue-filter-removed.xml`).
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] asfgit closed pull request #3172: ARTEMIS-2797 - Reset queue properties by unsetting them in broker.xml
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #3172:
URL: https://github.com/apache/activemq-artemis/pull/3172
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] michaelandrepearce commented on pull request #3172: ARTEMIS-2797 - Allow for removing current queue filter
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on pull request #3172:
URL: https://github.com/apache/activemq-artemis/pull/3172#issuecomment-641415268
Can a xml config update test be made to cover reload from xml.
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] michaelpearce-gain commented on a change in pull request #3172: ARTEMIS-2797 - Reset queue properties by unsetting them in broker.xml
Posted by GitBox <gi...@apache.org>.
michaelpearce-gain commented on a change in pull request #3172:
URL: https://github.com/apache/activemq-artemis/pull/3172#discussion_r444062892
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/QueueImpl.java
##########
@@ -3954,7 +3954,7 @@ public QueueConfiguration getQueueConfiguration() {
.setAddress(address)
.setId(id)
.setRoutingType(routingType)
- .setFilterString(filter.getFilterString())
+ .setFilterString(filter == null ? null : filter.getFilterString())
Review comment:
can you rebase, this is already fixed in master
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] jsmucr commented on pull request #3172: ARTEMIS-2797 - Allow for removing current queue filter
Posted by GitBox <gi...@apache.org>.
jsmucr commented on pull request #3172:
URL: https://github.com/apache/activemq-artemis/pull/3172#issuecomment-645475649
@michaelandrepearce There are no tests (apart from the one testing filters) validating the new behavior. There's still a lot of _fun_ waiting for 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] michaelpearce-gain commented on a change in pull request #3172: ARTEMIS-2797 - Allow for removing current queue filter
Posted by GitBox <gi...@apache.org>.
michaelpearce-gain commented on a change in pull request #3172:
URL: https://github.com/apache/activemq-artemis/pull/3172#discussion_r438123727
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/impl/PostOfficeImpl.java
##########
@@ -689,7 +689,10 @@ public QueueBinding updateQueue(QueueConfiguration queueConfiguration) throws Ex
queue.setDelayBeforeDispatch(queueConfiguration.getDelayBeforeDispatch().longValue());
}
Filter filter = FilterImpl.createFilter(queueConfiguration.getFilterString());
- if (filter != null && !filter.equals(queue.getFilter())) {
+ if ((filter == null) && (queue.getFilter() != null)) {
Review comment:
e.g. where we want to clear a filter currently we simply set it to empty string.
----------------------------------------------------------------
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