ARTEMIS-856 - Support consumersBeforeDispatch and delayBeforeDispatch #2175
Conversation
michaelandrepearce
force-pushed
the
ARTEMIS-856
branch
from
4ffcd16
to
86950a4
Compare
| @@ -270,6 +271,15 @@ | |||
|
|
|||
| private final QueueFactory factory; | |||
|
|
|||
| private final AtomicBoolean dispatching = new AtomicBoolean(); | |||
There was a problem hiding this comment.
The logic that involves using dispatching and dispatchStartTime could be simplyfied using just dispatchStartTime?
I would use and AtomicLongFieldUpdater for that in case
There was a problem hiding this comment.
I wanted that when dispatching the quickest eval is done, thus why the seperate boolean. As it is hot path. Agree could just use != -1 check, memory vs cpu cycle. Let me know your thought
Agree re atomic updater, i can make this change.
There was a problem hiding this comment.
Agree could just use != -1 check, memory vs cpu cycle
TBH to me are good enough both, but if we can just check !=-1 it would be welcome, even if less readable...
I'm more worried about the writes cost then the reads :P
There was a problem hiding this comment.
@franz1981 so after going back to see if i could just use != -1, i remembered why i couldnt it would mean adding a System.currentMillis call on the hot path, which is very costly.
I have though moved to using AtomicUpdaters , though :)
|
Fyi i go on annual leave 4pm tomorrow, so if youbcan comment before i can make changes before i leave else it will have to wait 2 weeks |
michaelandrepearce
force-pushed
the
ARTEMIS-856
branch
2 times, most recently
from
dbbb6ff
to
aae5493
Compare
|
note: seems test failure is related to #2173 |
michaelandrepearce
force-pushed
the
ARTEMIS-856
branch
from
aae5493
to
349e4c9
Compare
https://issues.apache.org/jira/browse/ARTEMIS-856 This is equivalent to consumersBeforeDispatchStarts and timeBeforeDispatchStarts in ActiveMQ 5.x http://activemq.apache.org/message-groups.html This is addressing one of the items on the artemis roadmap: http://activemq.apache.org/activemq-artemis-roadmap.html
michaelandrepearce
force-pushed
the
ARTEMIS-856
branch
from
349e4c9
to
1e6fe34
Compare
|
@franz1981 @clebertsuconic i will merge in the morning if no further comments |
|
Ohhh.. nice one.. I just looked at what was the semantics changed.. nice one!!!! it would be nice to add some docs.. a paragraph would do. If you don't have time to do it now I will merge it and do it later.. let me know. |
| * access control | ||
| */ | ||
| @CallerSensitive | ||
| public static <U> AtomicBooleanFieldUpdater<U> newUpdater(Class<U> tclass, String fieldName) { |
There was a problem hiding this comment.
I know that is a sad that JDK isn't providing this class with a native implementation, but I would use directly AtomicIntegerFiledUpdater or AtomicReferenceFieldUpdater with Boolean values instead.
The reason is that there is an implicit behaviour of any SomethingFiledUpdater that can't be enforced here: the declaration of a volatile Something.
Instead, the real purpose of this class is to treat integer values like boolean ones so I suppose that using a plain statitc util class to convert boolean<->integer will make it simple while avoiding to provide a more complex concurrent util class.
There was a problem hiding this comment.
@franz1981 isn't that too much nit picking? :)
I like the work as is.. as anything in life.. it can always be improved.. but the excellent is the enemy of the very good :)
There was a problem hiding this comment.
@franz1981 using Boolean you might as well then just use atomic boolean, which you commented eadlier on and rightly so to use atomic updater. As the point is to save having object reference overheads.
The point of the class is just to make it reusable if else where you want else you end up with lots of dupe code just for all the int to bool logic.
There was a problem hiding this comment.
I agree about the reasons but given that the JVM doesn't trust final fields you won't get the same performance you would have just using the int updater. Given that, I suppose that everything that let it works is ok for me: the PR is well done as always :)
|
@clebertsuconic if could merge as is, ill get docs done in another pr but before release if ok |
|
@michaelandrepearce I created https://issues.apache.org/jira/browse/ARTEMIS-1991 to track the doc. Marked as blocker just so it appears on the top of the list before release. I'm using that as a marker on what needs to be finished before we release. |
|
@michaelandrepearce this is broken... can you rebase and check what's happening? |
|
Ill rebase tomorrow |
|
this is so weird.. I have no idea what changed.. but I can't build on artemis-features. Probably the double xsd on tools? I 've tried debugging... and I can't figure out I will keep trying tomorrow. but if you know anything @michaelandrepearce please let me know. |
|
@clebertsuconic ill close this as i dont have time today or monday to dedicate further on this, as coming off holiday have a bit of a full plate in work, and ill work offline to see whats going and re-open once i figured it out. btw trying to ping you on IRC ;) |
|
Please don’t close it. If you don’t have time I will do it. It’s just I wanted to figure out Can you reopen please ? |
|
I already know this is because of AtomicBooleanFieldUpdater. Something is causing confusion on OSGI. |
https://issues.apache.org/jira/browse/ARTEMIS-856
This is equivalent to consumersBeforeDispatchStarts and timeBeforeDispatchStarts in ActiveMQ 5.x
http://activemq.apache.org/message-groups.html
This is addressing one of the items on the artemis roadmap: http://activemq.apache.org/activemq-artemis-roadmap.html