ARTEMIS-2076 Make Filter updateable #2296
Conversation
michaelandrepearce
force-pushed
the
2076
branch
5 times, most recently
from
94791d2
to
2f52060
Compare
| @@ -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, | |||
There was a problem hiding this comment.
We can't change the management API in anything but a major release. You'll need to add another method.
There was a problem hiding this comment.
This change is in the same release e.g. this is a new method for 2.7 (the delay before dispatch stuff is new e.g. this new method was added in this current cycle - #2175)
There was a problem hiding this comment.
Got it. Thanks for the clarification.
|
Is this updating the queue on the journal as well ? |
|
It is not !!! This will need to be a more complete feature. If you restart the broker you get the old filter back afaik. |
|
@clebertsuconic its updating as per other queue field update.... see updateQueue in postoffice (this was prexisiting and how other updates work) . which updates back to the storage manager. If thats not working (e.g. the update to storage isn't being honoured) then theres a bigger issue / regression in update queue bindings within the storagemanager thats pre-existing and seperate. what makes you think this isnt? if you could share, as this would affect other update use cases as noted, shares all same logic. |
michaelandrepearce
force-pushed
the
2076
branch
from
2f52060
to
01e8e7c
Compare
|
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). |
|
@jbertram yes, i was wondering about squashing and then worried about your commit, as you dont mind, ill squash. |
michaelandrepearce
force-pushed
the
2076
branch
from
01e8e7c
to
1c99d90
Compare
|
@jbertram all squashed now. |
michaelandrepearce
force-pushed
the
2076
branch
from
1c99d90
to
6bdd881
Compare
|
@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? |
|
@clebertsuconic the issue on vote thread is around route-type change from another JIRA and PR, this is separate feature around updating filters, 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. FYI on the the route-type change ill send in a PR as an alternative fix on that that also would avoid queue destruction. (the underlying issue is during deploy there's actually an error thrown due to routing on address info update before deploy of queues its trying to update the addressinfo before the queue route is changed, thus it wasn't actually getting to the update queue logic when you do a routing change) |
|
LGTM |
|
Dont merge i need to extend the tests still as per cleberts request |
|
@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? |
Add Tests Add implementation inline with other queue updatable settings. Enhance tests to ensure queue is not destroyed during config change and messages in queue already are preserved
michaelandrepearce
force-pushed
the
2076
branch
from
6bdd881
to
8dc86a3
Compare
|
@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. |
|
I was able to fix the conflicts myself after the previous merges.. don't worry! |
|
@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) |
|
Don’t worry. It was easy to rebase. Simple one. Will merge it soon. |
No description provided.