ARTEMIS-196 Implement Consumer Priority #2490
Conversation
|
@michaelandrepearce Nice! Will take a look today or max tomorrow 👍 |
michaelandrepearce
force-pushed
the
V2-196
branch
4 times, most recently
from
f0dd99c
to
6ddc595
Compare
|
@franz1981 just ignore class comments, theyre the originals still, ill need to change, but wanted to get to you quickly so you have chance to look over. If you think this is better ill make final tidyup bits, such as class comments and replace the real PR's branch. --update-- |
michaelandrepearce
force-pushed
the
V2-196
branch
2 times, most recently
from
34424b3
to
e6a6285
Compare
|
@franz1981 did you get a chance to look, do you think this is better than original solution? Am keen to get this feature into the next release cut. |
|
@michaelandrepearce sadly haven't had much time today to look into it :( |
|
@franz1981 great thanks a million :) |
...apache/activemq/artemis/core/protocol/core/impl/wireformat/SessionCreateConsumerMessage.java
Outdated
Show resolved
Hide resolved
...apache/activemq/artemis/core/protocol/core/impl/wireformat/SessionCreateConsumerMessage.java
Show resolved
Hide resolved
...ocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/broker/AMQPSessionCallback.java
Outdated
Show resolved
Hide resolved
...test/java/org/apache/activemq/artemis/tests/integration/jms/client/ConsumerPriorityTest.java
Outdated
Show resolved
Hide resolved
...va/org/apache/activemq/artemis/tests/integration/openwire/amq/QueueConsumerPriorityTest.java
Outdated
Show resolved
Hide resolved
...c/test/java/org/apache/activemq/artemis/tests/integration/amqp/AmqpReceiverPriorityTest.java
Show resolved
Hide resolved
...c/test/java/org/apache/activemq/artemis/tests/integration/amqp/AmqpReceiverPriorityTest.java
Show resolved
Hide resolved
...c/test/java/org/apache/activemq/artemis/tests/integration/amqp/AmqpReceiverPriorityTest.java
Outdated
Show resolved
Hide resolved
michaelandrepearce
force-pushed
the
V2-196
branch
2 times, most recently
from
1759d8d
to
fab1c1f
Compare
|
@gemmellr thanks for review, have updated this pr, if you could recheck the AMQPSessionCallback for me, to make sure i understood you basically we can expect any number to be representative of the priority. My understanding is an integral is a whole number, that essentially in java means any Number, but we still expect the whole number within the integer range aka -2^31 to 2^31-1. Also i added some additional tests to test for different number type representations of the integral. As for the Openwire test case, this was a simple port over of the existing activemq5 test case as untouched as possible, i agree we could reduce the time but id rather (at least for this release 2.7.0) keep it the same, so we can be sure feature works for openwire same as activemq5. |
michaelandrepearce
force-pushed
the
V2-196
branch
2 times, most recently
from
136a058
to
4e9bb8b
Compare
There was a problem hiding this comment.
I'm happy with this changes if compared with the previous PR, so kudos to @michaelandrepearce 👍
I think that given the new defined abstractions it is time to create some unit tests around the new collections/iterators etc etc to be sure in the future that if changed we will know how was the original contract that ATM (in the previous impl too) is only in the code.
I'm blaming myself for a recent PR that I will change after this review for this same reason I swear :P
artemis-commons/src/main/java/org/apache/activemq/artemis/utils/collections/MultiIterator.java
Outdated
Show resolved
Hide resolved
artemis-commons/src/main/java/org/apache/activemq/artemis/utils/collections/MultiIterator.java
Outdated
Show resolved
Hide resolved
artemis-commons/src/main/java/org/apache/activemq/artemis/utils/collections/MultiIterator.java
Outdated
Show resolved
Hide resolved
artemis-commons/src/main/java/org/apache/activemq/artemis/utils/collections/MultiIterator.java
Show resolved
Hide resolved
artemis-commons/src/main/java/org/apache/activemq/artemis/utils/collections/MultiIterator.java
Outdated
Show resolved
Hide resolved
...is-server/src/main/java/org/apache/activemq/artemis/core/server/impl/QueueConsumersImpl.java
Outdated
Show resolved
Hide resolved
...is-server/src/main/java/org/apache/activemq/artemis/core/server/impl/QueueConsumersImpl.java
Outdated
Show resolved
Hide resolved
...is-server/src/main/java/org/apache/activemq/artemis/core/server/impl/QueueConsumersImpl.java
Outdated
Show resolved
Hide resolved
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/QueueImpl.java
Show resolved
Hide resolved
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/QueueImpl.java
Show resolved
Hide resolved
...-commons/src/main/java/org/apache/activemq/artemis/utils/collections/PriorityCollection.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Yep, changing to Number to cover the integral types is good.
I think there are now a few more type-system test derivatives than need be, much faster unit tests would generally be better for covering things like that. Just hitting a few key points seems fine, e.g byte, int, an unsigned. I don't think it need test floating point types, I'd consider that an impl side effect more than anything.
...apache/activemq/artemis/core/protocol/core/impl/wireformat/SessionCreateConsumerMessage.java
Show resolved
Hide resolved
...va/org/apache/activemq/artemis/tests/integration/openwire/amq/QueueConsumerPriorityTest.java
Outdated
Show resolved
Hide resolved
...c/test/java/org/apache/activemq/artemis/tests/integration/amqp/AmqpReceiverPriorityTest.java
Show resolved
Hide resolved
|
@gemmellr ok will reduce back down the extra tests i added. To those you suggest. Also re activemq5. Fair enough, ill reduce / alter this then. |
|
@franz1981 good point around adding further tests to the extracted out bits. Agree it will make everything more robust |
michaelandrepearce
force-pushed
the
V2-196
branch
2 times, most recently
from
f21e150
to
c032311
Compare
|
@gemmellr pushed changes based on latest comments, thanks. |
|
@franz1981 pushed following changes based on your comments that agree with, for others i have left comment for us to discuss. Changes:
|
michaelandrepearce
force-pushed
the
V2-196
branch
4 times, most recently
from
10bb38c
to
effdc64
Compare
|
one test failure at least: JMSMessageGroupsTest The testsuite didn't complete.. but that one is failing for sure. |
|
I will spend some time understanding the failure and propose changes. I will communicate through here.. and if you find anything please let me know here as well. |
|
Ill have a look, i know that one was working as i had run those and exlusive test when i first pr'd i assume i broke something during changes during reviews |
|
@clebertsuconic ok so i know whats going on, another feature i have implemented for 2.7.0 reset of message groups, the test i had created is a little over strict and tested exact current behaviour, rather than what is just needed, that when resetting a message group, the selection on the next consumer for that group would have been the next consumer that joined. Where as actually when you reset the group in reality you just open the oppurtunity for another consumer to take the group. Im thinking i could add a repeat method to the iteration, so when a consumer it will repeat, so return the same consumer, this would give same as before. |
|
Sorted locally just extracting repeatble iterator logic as franz is keen on this |
michaelandrepearce
force-pushed
the
V2-196
branch
from
8937be8
to
149009f
Compare
|
@clebertsuconic sorted, just added ability in QueueConsumers to repeat (new repeatable iterator), and then if group, we tell it to repeat, that provides the original behavior, and test is passing locally. |
|
Current list of failures:
|
|
@clebertsuconic can you recheck this against master, i know things like testDoubleDelete are failing currently on master without this PR. |
michaelandrepearce
force-pushed
the
V2-196
branch
from
149009f
to
00fee69
Compare
|
@clebertsuconic the message group test ones are are genuine, and i have fixed so now should be fixed up in last push. I dont believe StompWebSocketMaxFrameTest is related, when i run this its erroring because |
|
@clebertsuconic could you look at this b3f0a87#r31982567, all builds are failing since that was merged to master..... |
|
@clebertsuconic once master is fixed ill rebase and if i could ask if you then could re run this? |
michaelandrepearce
force-pushed
the
V2-196
branch
3 times, most recently
from
108b40b
to
b0742dd
Compare
|
@clebertsuconic fully rebased (including over the FQQN for producers stuff) |
|
@clebertsuconic so as noted, the message groups ones is sorted, repeat needs to occur only if groupconsumer exist. It does seem MQTTTest and StompWebSocketMaxFrameTest is directly broken due to this, but i cant think to why, any ideas welcome, ill continue to look into |
Add consumer priority support Includes refactor of consumer iterating in QueueImpl to its own logical class, to be able to implement. Add OpenWire JMS Test - taken from ActiveMQ5 Add Core JMS Test Add AMQP Test Add Docs
|
@clebertsuconic ive found the pesky thing, in ServerSessionImpl where i had added a new parameter to the createConsumer method, the old method signature where we pass though but default the priority in the new, i had missed forwarding on the credits value, thus the issue. Typical one liner causing lots of headache ;) |
michaelandrepearce
force-pushed
the
V2-196
branch
from
b0742dd
to
08a33ab
Compare
|
@clebertsuconic hows this looking now, we good to merge??? |
|
Let me run one final round of testsuite. If all good will merge it tomorrow. |
|
It’s important to keep the testsuite clean. As we should release soon. |
|
@michaelandrepearce I'm pretty sure I will be able to merge this tomorrow. Testsuite had some unrelated failures. I just want to double check.. if there's any issues I'm sure i will be able to figure out. will let you know tomorrow. I got the results pretty late after I finished. leave it with me for tomorrow first thing please? |
|
I added one change on top of yours during merge... to remove unused variables.. merged! you are the man! |
|
@clebertsuconic some of those variables are used but accesed by atomic updaters. Have commented on your commit |
|
@michaelandrepearce duh.. thanks |
@franz1981 an alternative so we don't have to have a copy of CopyOnWriteArrayList, it does mean on add or remove consumer we have to invoke toArray which causes a copy, but this is not on hot path, so i think we should be good, and avoids us having to clone a jvm class.