Join GitHub today
GitHub is home to over 40 million developers working together to host and review code, manage projects, and build software together.
Sign up[WFLY-10351] BMT interceptor timeout reset adjustment #12225
Conversation
ochaloup
changed the title
| @@ -60,7 +59,7 @@ public Object processInvocation(final InterceptorContext context) throws Excepti | |||
| if (oldTx != null) tm.resume(oldTx); | |||
| } | |||
| } finally { | |||
| tm.setTransactionTimeout(oldTimeout == -1 ? 0 : oldTimeout); | |||
| tm.setTransactionTimeout(oldTimeout == ContextTransactionManager.getGlobalDefaultTransactionTimeout() ? 0 : oldTimeout); | |||
This comment has been minimized.
This comment has been minimized.
bstansberry
Apr 13, 2019
Contributor
The WFLY-10146 change included the following comment before this line, which I'd like to see here as well as it helps clarify what's otherwise pretty confusing:
// See also https://issues.jboss.org/browse/WFTC-44
The part of the WFLY-10146 change adding that comment:
https://github.com/wildfly/wildfly/pull/11086/files#diff-e05009bda935bba48b4ee7ca633ba1ecR170
The rest of the changes in this class all LGTM; they are just changing to standard ContextTransactionManager use. I think this line seems fine too (other than needing the comment) but checking with David or @chengfang or @tadamski makes sense.
This comment has been minimized.
This comment has been minimized.
dmlloyd
Apr 13, 2019
Member
I agree with Brian, adding the comment would be a good idea. Otherwise IIRC this seems correct.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
ochaloup
Apr 17, 2019
•
Author
Contributor
@kabir just I'm now not sure if you refer to something more which is missing.
I added the comment // See also https://issues.jboss.org/browse/WFTC-44 above the change of the tm.setTransactionTimeout(...). I can see the change is still at the place if I take a look to the PR "Files changes". Could be that only the github does not show this as "outdated" as I did not do any change on the highlighted line but above it.
This comment has been minimized.
This comment has been minimized.
adding tests for timeout issue the XAResource setTransactionTimeout: Once set, this timeout value is effective until setTransactionTimeout is invoked again with a different value.
ochaloup
force-pushed the
ochaloup:WFLY-10351-btm-interceptor-cleanup-timeout-issue
branch
from
1c53066
to
098376c
This comment has been minimized.
This comment has been minimized.
|
@bstansberry @dmlloyd thanks. I added the comment |
kabir
merged commit c3e8825
into
wildfly:master
ochaloup commentedApr 12, 2019
•
edited
https://issues.jboss.org/browse/WFLY-10351
Adding correct transaction timeout handling for propagate the reset of the timeout to the WFTC and not to Narayana. Adding tests on fact that the method setTransactionTimeout() sets the timeout value for thread being active.
@dmlloyd would you be so kind and check if this follows the rationale the issue was created with and how the WFLY-10146 was treated. Thanks.