[DRIVERS-2104] The transaction spec says to unpin after MaxTimeMSExpired on commit but there's a test for the opposite behavior Created: 12/Jun/19 Updated: 31/Mar/22 |
|
| Status: | Backlog |
| Project: | Drivers |
| Component/s: | Transactions |
| Fix Version/s: | None |
| Type: | Spec Change | Priority: | Major - P3 |
| Reporter: | Shane Harvey | Assignee: | Unassigned |
| Resolution: | Unresolved | Votes: | 0 |
| Labels: | None | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Issue Links: |
|
||||||||||||||||
| Driver Changes: | Needed | ||||||||||||||||
| Description |
|
The spec says:
And MaxTimeMSExpired is defined as an UnknownTransactionCommitResult error. So a MaxTimeMSExpired error when running commitTransaction should unpin the session. However the "remain pinned after non-transient error on commit" test asserts the opposite behavior; that a session remains pinned after a MaxTimeMSExpired error on commit. |
| Comments |
| Comment by Oleg Pudeyev (Inactive) [ 27/Dec/19 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
In https://jira.mongodb.org/browse/SPEC-1378, the test which used to assert that maxtimems error did not unpin was changed to use a different error (and still not unpin), because maxtimems error unpinned. I believe we are all in agreement that the new test is correct. There is also a test for unpinning on a transient error. This test uses a network error (via fail point) to obtain the transient error. vincent.kam proposed that a test be added for maxtimems operation failure, which would also unpin. Although in principle the same code path in the driver should handle an operation failure with transient label and a network error, for the purposes of unpinning, I would say the maxtimems test is meaningfully different from the integration standpoint and I support adding it. For convenience of reference this is the test that Vincent proposed:
I also agree with shane.harvey's second point that maxtimemsexpired should not retry because doing so would double the timeout, though as I understand it this would be a spec change and not just a test change. | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Vincent Kam (Inactive) [ 12/Dec/19 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
I agree with Shane. The .NET Driver also ran into this ambiguity while implementing | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Shane Harvey [ 12/Dec/19 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
I think this ticket still needs some work. The spec is ambiguous about these two points:
For 1, the answer is yes. MaxTimeMSExpired errors on commit are labeled UnknownTransactionCommitResult which will cause the session to be unpinned. We should add a test for this. For 2, the answer should be no. We should not retry after MaxTimeMSExpired error on commit because it would implicitly double the application’s maxCommitTimeMS. We should add a test to cover this case. It will be similar to the "commit is not retried after MaxTimeMSExpired error" withTransaction test. | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Jeffrey Yemin [ 26/Nov/19 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
shane.harvey is this ticket superseded by DRIVERS-764? | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Githook User [ 12/Aug/19 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Author: {'name': 'Dan Aprahamian', 'email': 'dan.aprahamian@gmail.com', 'username': 'daprahamian'}Message: test(transactions): disable invalid pinning test Disables test 'remain pinned after non-transient error on commit' |