ARTEMIS-196 Implement Consumer Priority

[michaelandrepearce]

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

[michaelandrepearce]

Ready for review.

Ill look to merge within the week.

As noted by the Jira ticket, this is to add feature that is in ActiveMQ5 (http://activemq.apache.org/consumer-priority.html) so part of bringing feature parity, also note feature is in RabbitMQ also.

Think this might be the lowest Jira Ticket number i've worked on :)
https://issues.apache.org/jira/browse/ARTEMIS-196

[franz1981]

I have taken a fast look and already written down some comment: it looks ok to me, but I need to look further in the QueueConsumersImpl details.
I'm happy about the nice abstraction over the consumers to simplify the message handling on QueueImpl and I suppose that keeping refactoring and this feature (consumer's priorities) separated isn't easy (nor sure it makes sense TBH), so I won't bother asking for it. well done! 👍

[michaelandrepearce]

@franz1981 alot of comments where why integer vs byte, whilst legacy ActiveMQ only supported 0-127 in open wire, many other brokers support integer for this feature, e.g. QPID for AMQP supports -2^31 to 2^31-1 like wise rabbitmq.

e.g.
Qpid -https://qpid.apache.org/releases/qpid-broker-j-7.0.6/book/Java-Broker-Runtime-Consumers.html#Java-Broker-Runtime-Consumers-Prioirty
"In AMQP 1.0 the priority of the consumer is set in the properties map of the attach frame where the broker side of the link represents the sending side of the link. The key for the entry must be the literal string priority, and the value of the entry must be an integral number in the range -2^31 to 2^31-1."

RabbitMQ - https://www.rabbitmq.com/consumer-priority.html "Set the x-priority argument in the basic.consume method to an integer value"

This is set on the consumer (its not per message) so size isn't an issue it makes sense to support the int and not constrain our selves un-neededly, making people migrating AMQP easier.

[franz1981]

@michaelandrepearce Yep, I have written it everytime while reading the code mate, sorry :)
Yes I do understand the bits related to byte vs int (and I agree) and related to Iterator:.reset too (i agree there too).
Just I'm concerned about reducing as much as possible the code complexity by having "inherited" a custom "CopyOnWriteArrayList".
I can't say if is better to simplify it as much as possible to get what we really need or just having this custom version of it as we are doing now.
Using a final static MethodHandle to steal the array when we need it could be not such a bad solution too: I'm not a fan of reflection-magic-like things but I admit that it could avoid to copy an entire implementation just for 1 or 2 missing methods.

[michaelandrepearce]

Reflection would not be good here, getArray is HOT path, its reason why i need it. Also it wont solve all issues, if you see i dont actually synconize in the Level, i removed this, as we have the sync only at the top level add, remove methods, thus saving extra locks that we would have if just extended CopyOnWriteArrayList.

We still need a custom implementation as we need the array of levels, its just the design principles is based on CopyOnWriteArrayList, e.g. modifications modify the array by copying not changing the existing array, and all read access uses the existing arrays to avoid reads needing copies. E.g. read path = hotpath, modification is not.

[franz1981]

@michaelandrepearce Consider that a static final MethodHandle::invokeExact has performance similar to a direct call, not a reflective call http://mail.openjdk.java.net/pipermail/jigsaw-dev/2017-January/010894.html

[michaelandrepearce]

@franz1981 is that java 8? I thought it was Java 9+

[franz1981]

@michaelandrepearce It has been introduced into Java 8 to support the lambda as they are now, Java 9 has introduced VarHandle, that has a similar name and overlapping functionalities :P

[michaelandrepearce]

@franz1981 can you check your private email, a few queries on MethodHandle ;) especially on a accessing method thats private/package but where the JVM doesnt allow reflection access override, due to security manager policy settings.

[franz1981]

Yep, it is a common scenario to use a security manager so I prefer to not force adding any exception on the security manger to allow just this calls to work

[michaelandrepearce]

Cool so will leave as is then. thanks for the review!

[michaelandrepearce]

Im going to close this for the other PR #2490