-
Notifications
You must be signed in to change notification settings - Fork 1.1k
INT-4317: JMS: dynamic deliverMode and timeToLive #2561
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple of comments/questions.
build.gradle
Outdated
@@ -1,5 +1,5 @@ | |||
buildscript { | |||
ext.kotlinVersion = '1.2.51' | |||
ext.kotlinVersion = '1.2.60' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
61 is out already.
testCompile("com.willowtreeapps.assertk:assertk:$assertkVersion") { | ||
exclude group: 'org.jetbrains.kotlin', module: 'kotlin-reflect' | ||
} | ||
testCompile("com.willowtreeapps.assertk:assertk-jvm:$assertkVersion") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the exclusion no longer needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According my Gradle report the old version is remapped pretty well:
+--- com.willowtreeapps.assertk:assertk-jvm:0.12
| +--- com.willowtreeapps.assertk:assertk-common:0.12
| | +--- org.jetbrains.kotlin:kotlin-stdlib-common:1.2.50 -> 1.2.61
| | \--- com.willowtreeapps.opentest4k:opentest4k-common:1.1.0
| | \--- org.jetbrains.kotlin:kotlin-stdlib-common:1.2.41 -> 1.2.61
| +--- org.jetbrains.kotlin:kotlin-stdlib:1.2.50 -> 1.2.61
| | +--- org.jetbrains.kotlin:kotlin-stdlib-common:1.2.61
| | \--- org.jetbrains:annotations:13.0
| +--- org.jetbrains.kotlin:kotlin-reflect:1.2.50 -> 1.2.61
| | \--- org.jetbrains.kotlin:kotlin-stdlib:1.2.61 (*)
| \--- com.willowtreeapps.opentest4k:opentest4k-jvm:1.1.0
| +--- com.willowtreeapps.opentest4k:opentest4k-common:1.1.0 (*)
| +--- org.jetbrains.kotlin:kotlin-stdlib-jdk8:1.2.41 -> 1.2.61
| | +--- org.jetbrains.kotlin:kotlin-stdlib:1.2.61 (*)
| | \--- org.jetbrains.kotlin:kotlin-stdlib-jdk7:1.2.61
| | \--- org.jetbrains.kotlin:kotlin-stdlib:1.2.61 (*)
| \--- org.opentest4j:opentest4j:1.1.0
+--- org.jetbrains.kotlin:kotlin-stdlib-jdk8:1.2.61 (*)
And there is no any issues during running tests against that latest AssertK.
} | ||
finally { | ||
DynamicJmsTemplateProperties.clearPriority(); | ||
DynamicJmsTemplateProperties.clearDeliveryMode(); | ||
DynamicJmsTemplateProperties.clearTimeToLive(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if it would be better to have one ThreadLocal with a container?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what is your concern: we have a container there and it is a ThreadLocalMap
:
public void set(T value) {
Thread t = Thread.currentThread();
ThreadLocalMap map = getMap(t);
if (map != null)
map.set(this, value);
else
createMap(t, value);
}
So, essentially with your suggestion we are going to have only a single entry in this map, but we would create that extra container anyway.
And at the same time we would lose a flexibility when we deal only with one property. For example a setPriority()
is used from the AbstractJmsChannel
, too.
So, what am I missing staying with the standard Java approach?
Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, we now have 3 ThreadLocal
s.
I was just suggesting one ThreadLocal<SomeContainer>
and one DynamicJmsTemplateProperties.clear()
method.
On the other hand, we'd create/remove a new Object for each send, so I suppose the current is better (since the ThreadLocalMap
is retained).
So, OK, LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh? No, there is a single map-per-thread: ThreadLocalMap map = getMap(t);
. As I said: essentially it is the same container as you suggest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't mean to say there wasn't a map per thread. My main point was we could replace
DynamicJmsTemplateProperties.clearPriority();
DynamicJmsTemplateProperties.clearDeliveryMode();
DynamicJmsTemplateProperties.clearTimeToLive();
with just one DynamicJmsTemplateProperties.clear()
method.
But, as I said, I am ok with this.
JIRA: https://jira.spring.io/browse/INT-4317 * Add `deliveryModeExpression` and `timeToLiveExpression` properties to the `JmsSendingMessageHandler` and expose them in the Java DSL and XML components * Add `setMapInboundDeliveryMode()` and `setMapInboundExpiration()` `boolean` properties (default `false`) to the `DefaultJmsHeaderMapper` for transferring `JMSDeliveryMode` and `JMSExpiration` into appropriate `JmsHeaders.DELIVERY_MODE` and `JmsHeaders.EXPIRATION` headers * Upgrade to latest Kotlin and AssertK
2cb5046
to
35749ec
Compare
JIRA: https://jira.spring.io/browse/INT-4317
deliveryModeExpression
andtimeToLiveExpression
propertiesto the
JmsSendingMessageHandler
and expose them in the Java DSL andXML components
setMapInboundDeliveryMode()
andsetMapInboundExpiration()
boolean
properties (defaultfalse
) to theDefaultJmsHeaderMapper
for transferring
JMSDeliveryMode
andJMSExpiration
into appropriateJmsHeaders.DELIVERY_MODE
andJmsHeaders.EXPIRATION
headers