You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@activemq.apache.org by michaelandrepearce <gi...@git.apache.org> on 2018/09/06 07:59:02 UTC
[GitHub] activemq-artemis pull request #2296: ARTEMIS-2076 Make Filter updateable
GitHub user michaelandrepearce opened a pull request:
https://github.com/apache/activemq-artemis/pull/2296
ARTEMIS-2076 Make Filter updateable
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/michaelandrepearce/activemq-artemis 2076
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/activemq-artemis/pull/2296.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #2296
----
commit 88b03506dc77b4c87a7ca4a63843350c2617ead8
Author: Michael André Pearce <mi...@...>
Date: 2018-09-06T07:50:16Z
ARTEMIS-2076 Make Filter updateable
----
---
[GitHub] activemq-artemis issue #2296: ARTEMIS-2076 Make Filter updateable
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/2296
@clebertsuconic i have enhanced the tests to check binding ids are the same (aka not recreated) and also added check for message loss, this is added both to the redeploy and the restart test cases.
---
[GitHub] activemq-artemis pull request #2296: ARTEMIS-2076 Make Filter updateable
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/activemq-artemis/pull/2296
---
[GitHub] activemq-artemis issue #2296: ARTEMIS-2076 Make Filter updateable
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/2296
if (filter != null && !filter.equals(queue.getFilter())) {
changed = true;
queue.setFilter(filter);
}
if (logger.isDebugEnabled()) {
if (user == null && queue.getUser() != null) {
logger.debug("Ignoring updating Queue to a NULL user");
}
}
if (user != null && !user.equals(queue.getUser())) {
changed = true;
queue.setUser(user);
}
if (changed) {
final long txID = storageManager.generateID();
try {
storageManager.updateQueueBinding(txID, queueBinding);
storageManager.commitBindings(txID);
} catch (Throwable throwable) {
storageManager.rollback(txID);
logger.warn(throwable.getMessage(), throwable);
throw throwable;
}
}
---
[GitHub] activemq-artemis issue #2296: ARTEMIS-2076 Make Filter updateable
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on the issue:
https://github.com/apache/activemq-artemis/pull/2296
Is this updating the queue on the journal as well ?
---
[GitHub] activemq-artemis pull request #2296: ARTEMIS-2076 Make Filter updateable
Posted by jbertram <gi...@git.apache.org>.
Github user jbertram commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2296#discussion_r215657277
--- Diff: artemis-core-client/src/main/java/org/apache/activemq/artemis/api/core/management/ActiveMQServerControl.java ---
@@ -710,6 +710,7 @@ String updateQueue(@Parameter(name = "name", desc = "Name of the queue") String
@Parameter(name = "exclusive", desc = "If the queue should route exclusively to one consumer") Boolean exclusive,
@Parameter(name = "consumersBeforeDispatch", desc = "Number of consumers needed before dispatch can start") Integer consumersBeforeDispatch,
@Parameter(name = "delayBeforeDispatch", desc = "Delay to wait before dispatching if number of consumers before dispatch is not met") Long delayBeforeDispatch,
+ @Parameter(name = "filter", desc = "The filter to use on the queue") String filter,
--- End diff --
Got it. Thanks for the clarification.
---
[GitHub] activemq-artemis issue #2296: ARTEMIS-2076 Make Filter updateable
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/2296
Dont merge i need to extend the tests still as per cleberts request
---
[GitHub] activemq-artemis pull request #2296: ARTEMIS-2076 Make Filter updateable
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2296#discussion_r215631874
--- Diff: artemis-core-client/src/main/java/org/apache/activemq/artemis/api/core/management/ActiveMQServerControl.java ---
@@ -710,6 +710,7 @@ String updateQueue(@Parameter(name = "name", desc = "Name of the queue") String
@Parameter(name = "exclusive", desc = "If the queue should route exclusively to one consumer") Boolean exclusive,
@Parameter(name = "consumersBeforeDispatch", desc = "Number of consumers needed before dispatch can start") Integer consumersBeforeDispatch,
@Parameter(name = "delayBeforeDispatch", desc = "Delay to wait before dispatching if number of consumers before dispatch is not met") Long delayBeforeDispatch,
+ @Parameter(name = "filter", desc = "The filter to use on the queue") String filter,
--- End diff --
This change is in the same release e.g. new method (the delay before dispatch stuff is new)
---
[GitHub] activemq-artemis issue #2296: ARTEMIS-2076 Make Filter updateable
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/2296
@clebertsuconic i can rebase if you need to merge this? I have one locally already, let me know if you wish me to push up, or if you're merging as is (i assume from your response the latter)
---
[GitHub] activemq-artemis issue #2296: ARTEMIS-2076 Make Filter updateable
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/2296
@clebertsuconic the issue on vote thread is around route-type change from another JIRA and PR, this is separate feature, though we're avoiding the issue that introduced, by following the same update logic of other queue field updates.
I will add a test case though to ensure when running and config is reloaded the queue is not destroyed but updated instead e.g. ensure the queue instance is the same.
---
[GitHub] activemq-artemis issue #2296: ARTEMIS-2076 Make Filter updateable
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/2296
@jbertram yes, i was wondering about squashing and then worried about your commit, as you dont mind, ill squash.
---
[GitHub] activemq-artemis issue #2296: ARTEMIS-2076 Make Filter updateable
Posted by jbertram <gi...@git.apache.org>.
Github user jbertram commented on the issue:
https://github.com/apache/activemq-artemis/pull/2296
LGTM
---
[GitHub] activemq-artemis issue #2296: ARTEMIS-2076 Make Filter updateable
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/2296
@jbertram all squashed now.
---
[GitHub] activemq-artemis issue #2296: ARTEMIS-2076 Make Filter updateable
Posted by jbertram <gi...@git.apache.org>.
Github user jbertram commented on the issue:
https://github.com/apache/activemq-artemis/pull/2296
IMO all these commits should be squashed. I don't mind if you take authorship of the test I wrote for the other PR (assuming you don't).
---
[GitHub] activemq-artemis issue #2296: ARTEMIS-2076 Make Filter updateable
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/2296
@clebertsuconic its updating as per other queue field update.... see updateQueue in postoffice.
---
[GitHub] activemq-artemis issue #2296: ARTEMIS-2076 Make Filter updateable
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on the issue:
https://github.com/apache/activemq-artemis/pull/2296
@michaelandrepearce if you don't have time to do it don't worry I will do it.. I was asking more in a way like.. if you can... know what I mean?
---
[GitHub] activemq-artemis issue #2296: ARTEMIS-2076 Make Filter updateable
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on the issue:
https://github.com/apache/activemq-artemis/pull/2296
@michaelandrepearce as I talked on the vote thread: Can you tweak (or add a test) that play with a broker restart, and validate the queue is updated on the journal, and messages not lost please?
---
[GitHub] activemq-artemis issue #2296: ARTEMIS-2076 Make Filter updateable
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on the issue:
https://github.com/apache/activemq-artemis/pull/2296
I was able to fix the conflicts myself after the previous merges.. don't worry!
---
[GitHub] activemq-artemis issue #2296: ARTEMIS-2076 Make Filter updateable
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on the issue:
https://github.com/apache/activemq-artemis/pull/2296
It is not !!! This will need to be a more complete feature. If you restart the broker you get the old filter back afaik.
---
[GitHub] activemq-artemis issue #2296: ARTEMIS-2076 Make Filter updateable
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on the issue:
https://github.com/apache/activemq-artemis/pull/2296
Don’t worry. It was easy to rebase. Simple one.
Will merge it soon.
---
[GitHub] activemq-artemis pull request #2296: ARTEMIS-2076 Make Filter updateable
Posted by jbertram <gi...@git.apache.org>.
Github user jbertram commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2296#discussion_r215621762
--- Diff: artemis-core-client/src/main/java/org/apache/activemq/artemis/api/core/management/ActiveMQServerControl.java ---
@@ -710,6 +710,7 @@ String updateQueue(@Parameter(name = "name", desc = "Name of the queue") String
@Parameter(name = "exclusive", desc = "If the queue should route exclusively to one consumer") Boolean exclusive,
@Parameter(name = "consumersBeforeDispatch", desc = "Number of consumers needed before dispatch can start") Integer consumersBeforeDispatch,
@Parameter(name = "delayBeforeDispatch", desc = "Delay to wait before dispatching if number of consumers before dispatch is not met") Long delayBeforeDispatch,
+ @Parameter(name = "filter", desc = "The filter to use on the queue") String filter,
--- End diff --
We can't change the management API in anything but a major release. You'll need to add another method.
---