Skip to content
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

GODRIVER-1852 Add commitTransaction error check for timeout #569

Merged
merged 8 commits into from
Feb 26, 2021
Merged

GODRIVER-1852 Add commitTransaction error check for timeout #569

merged 8 commits into from
Feb 26, 2021

Conversation

benjirewis
Copy link
Contributor

GODRIVER-1852

Adds a check for network timeout after executing the operation within a commitTransaction. Avoids incorrectly updating transaction state to "Committed" in the case of a timeout.

Adds test to make sure a transaction can still be aborted after a timed-out commitTransaction.

@benjirewis benjirewis marked this pull request as ready for review January 29, 2021 14:43
Copy link
Contributor

@kevinAlbs kevinAlbs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for investigating this! I have a question about the UnknownTransactionCommitResult error label which we discussed offline.

mongo/with_transactions_test.go Show resolved Hide resolved
mongo/with_transactions_test.go Show resolved Hide resolved
mongo/with_transactions_test.go Outdated Show resolved Hide resolved
@benjirewis benjirewis requested a review from kevinAlbs January 29, 2021 22:29
Copy link
Contributor

@kevinAlbs kevinAlbs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Brilliant work investigating and testing this! I just have a suggestion about the timeout check.

mongo/session.go Show resolved Hide resolved
mongo/session.go Outdated Show resolved Hide resolved
mongo/with_transactions_test.go Show resolved Hide resolved
mongo/with_transactions_test.go Show resolved Hide resolved
mongo/with_transactions_test.go Outdated Show resolved Hide resolved
@benjirewis benjirewis requested a review from kevinAlbs February 1, 2021 16:36
Copy link
Contributor

@kevinAlbs kevinAlbs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

mongo/with_transactions_test.go Show resolved Hide resolved
mongo/errors.go Outdated Show resolved Hide resolved
mongo/session.go Outdated Show resolved Hide resolved
mongo/session.go Outdated Show resolved Hide resolved
mongo/with_transactions_test.go Outdated Show resolved Hide resolved
mongo/with_transactions_test.go Show resolved Hide resolved
mongo/with_transactions_test.go Outdated Show resolved Hide resolved
mongo/with_transactions_test.go Outdated Show resolved Hide resolved
mongo/with_transactions_test.go Show resolved Hide resolved
mongo/with_transactions_test.go Outdated Show resolved Hide resolved
mongo/with_transactions_test.go Show resolved Hide resolved
Copy link
Contributor

@divjotarora divjotarora left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code changes LGTM. Just one additional note about leaving a comment to clarify what the code is doing for future readers. Additionally, can you rebase this branch on master and force-push? I see some test failures related to connection pooling and Atlas Data Lake, but these should be fixed by some of our recent commits. Once that's addressed, I can come back and approve.

mongo/session.go Show resolved Hide resolved
@benjirewis benjirewis merged commit 1c2620e into mongodb:master Feb 26, 2021
@benjirewis benjirewis deleted the addCommitTransactionErrorCheck.1852 branch February 26, 2021 18:45
This was referenced Mar 14, 2021
faem pushed a commit to kubedb/mongo-go-driver that referenced this pull request Mar 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants