Allow setting transaction timeout from a config property #15832
Conversation
quarkus-bot
bot
added
area/hibernate-orm
geoand
removed
area/hibernate-orm
...yana-jta/runtime/src/main/java/io/quarkus/narayana/jta/runtime/TransactionConfiguration.java
Outdated
Show resolved
Hide resolved
| Optional<Integer> configTimeout = ConfigProvider.getConfig() | ||
| .getOptionalValue(configAnnotation.timeoutFromConfigProperty(), Integer.class); |
There was a problem hiding this comment.
This needs to be set in a cache somewhere as acessing the config programmatively is very costly so you don't want to do it on each interceptor call.
There was a problem hiding this comment.
Hm... I am not sure we want to go there TBH because you would need a map from TransactionConfiguration to the config value.
That map would need to be a ConcurrentHashmap (which is fast, but the same as using a regular HashMap) and also we would need to make sure we expire entries to avoid having the map grow indefinitely.
I doubt that complexity is warranted. @gsmet WDYT?
There was a problem hiding this comment.
We already discussed with Stuart the need to have a cache to avoid getting the annotation via reflection on each call.
My experience is that retrieving a configuration item via ConfigProvider.getConfig() is really costly, not everything is in cache inside the configuration provider. I don't remember the detail but I know I already chased some of them because it shows up in some profile.
I would not be concerned about a possible risk of indefinitly growing map as I expect having a finite number of classes/method and very low number of annotation usage. What can be done is to compute it at build time and fallback to reflection if the cache is not populated, this is what I did for property mapping for MongoDB with Panache for ex to avoid too many reflection at runtime.
There was a problem hiding this comment.
Well, I'll be honest. I am certainly not going to do that now :). If someone else wants to take up the PR from here, cool :)
quarkus-bot
bot
added
area/hibernate-orm
|
Closing in favor of #17357 |
Resolves: #15752