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
WFLY-850 Adding transaction timeout info to programmatic timer #6088
Conversation
Build 3073 is now running using a merge of d8e3f12 |
@@ -100,6 +100,11 @@ public SessionBeanComponentCreateService(final ComponentConfiguration componentC | |||
processTxAttr(sessionBeanComponentDescription, MethodIntf.TIMER, method); | |||
} | |||
} | |||
if (sessionBeanComponentDescription.getTimeoutMethod() != null) { | |||
this.processTxAttr(sessionBeanComponentDescription, MethodIntf.TIMER, | |||
sessionBeanComponentDescription.getTimeoutMethod()); |
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.
Is there a chance that the same method might have processTxAttr() called on it twice, and if so, is there protection in that method to avoid any kind of duplication nonsense?
Patch otherwise looks reasonable to me.
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.
Method processTxAttr() adds information about attributes and timeouts to maps. Keys in those maps are objects of class MethodTransactionAttributeKey. This class contains information about both method and context of invocation. So there is no protection against method being called many times, but running it many times will leave those maps in the same state. On the other hand, I see one more problem here. Before my fix transaction timeouts were added to public @timeout methods with context other than timer and it didn't prevented information about timeout to be propagated to those methods when they were executed from timer context.
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.
Is this patch OK to merge as is, or do you want to try and verify/fix this second issue?
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 think that this patch can be merged because the second issue is independent. I can leave the Jira open and investigate it further. If it required fix then I would prepare another patch.
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.
OK in that case this has my +1
Build 3073 outcome was SUCCESS using a merge of d8e3f12 |
Merged |
No description provided.