[WFLY-10434] Make interceptor order arrangements for CachedConnection… #11335
Conversation
| if (!ejbSetupActions.isEmpty()) { | ||
| configuration.addTimeoutViewInterceptor(AdditionalSetupInterceptor.factory(ejbSetupActions), InterceptorOrder.View.EE_SETUP); | ||
| for (SetupAction setupAction : ejbSetupActions) { | ||
| configuration.addTimeoutViewInterceptor(new ImmediateInterceptorFactory(new AdditionalSetupInterceptor(setupAction)), setupAction.priority() == 0? InterceptorOrder.View.EE_SETUP : setupAction.priority()); |
There was a problem hiding this comment.
I don't really like this. Interceptor priorities have to be unique, so this has the potential to fail if the setup actions have the same priority, and I don't think it is really clear what is going on.
Can't we just change the InterceptorOrder.View.EE_SETUP priority to be inside the transaction? Looking at the code the only other subsystem to use this is the EE concurrency stuff, and I think that should not matter if it is executed inside the transaction.
There was a problem hiding this comment.
@stuartwdouglas I was afraid of having future consequences if I changed the priority value, because I was unsure what were the reasons, if any, for having EE_SETUP priority set to its current value. If you think it is okay, I'll do as you're suggesting. Thanks!
…t CachedConnectionManager setup action is invoked inside the transaction context
|
Retest this please |
|
@stuartwdouglas the requested changes are done and all checks are passing. Thanks for your review! |
…ManagerSetupProcessor setup action, making sure it runs after CMTTxInterceptor
Jira: https://issues.jboss.org/browse/WFLY-10434
@dmlloyd please review, what do you think of using the already existent priority() for this?
@stuartwdouglas any opinion on this is also welcome, since you wrote the SetupAction code.
Thanks!