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-2651 Break NoWritesPerformed-Only Error Sequence #1135

Merged
merged 19 commits into from Dec 8, 2022

Conversation

prestonvasquez
Copy link
Collaborator

GODRIVER-2651

Summary

When a "NoWritesPerformed" label is encountered, and the executor is out of retries, then we need to reset the error to the "previousIndefiniteError" and re-run the error check. This also needs to work for cases where the only errors that are returned are labeled "NoWritesPerformed"

Background & Motivation

There are two issues that were noted:

  1. If a sequence of retry errors all contain a "NoWritesPerformed" label, then the executor will propagate "nil" for the error value. This will lead to end-users assuming successful writes due to no errors, when in reality there were errors.
  2. Need to fully process "previousIndefiniteErrors". This can be correctly done by setting the "err" value to the previousIndefiniteError and then re-running the error check switch block.

x/mongo/driver/errors.go Outdated Show resolved Hide resolved
x/mongo/driver/operation.go Outdated Show resolved Hide resolved
@benjirewis benjirewis self-requested a review December 6, 2022 21:50
if tt.HasErrorLabel(NoWritesPerformed) && !errors.Is(tt, prevIndefiniteErr) {
err = prevIndefiniteErr

goto checkError
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For clarity, this "goto" is defensive. One of the conditions for setting the "prevIndefiniteError" is that it is not the same type of error as the one that is the subject of this switch condition: "tt". If that is the case, we need to repeat the switch block for the "previousIndefiniteError" so that it can run the logic for the correct case.

If the "previousIndefiniteError" is the same as "tt", then there is no need to entire this block and the error can continue with its processing.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thank you for the explanation. It makes sense.

x/mongo/driver/operation.go Outdated Show resolved Hide resolved
x/mongo/driver/operation.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@matthewdale matthewdale left a comment

Choose a reason for hiding this comment

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

Looks good 👍

Copy link
Collaborator

@qingyang-hu qingyang-hu left a comment

Choose a reason for hiding this comment

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

It looks great to me!

x/mongo/driver/operation.go Outdated Show resolved Hide resolved
if tt.HasErrorLabel(NoWritesPerformed) && !errors.Is(tt, prevIndefiniteErr) {
err = prevIndefiniteErr

goto checkError
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thank you for the explanation. It makes sense.

Co-authored-by: Qingyang Hu <103950869+qingyang-hu@users.noreply.github.com>
Copy link
Contributor

@benjirewis benjirewis left a comment

Choose a reason for hiding this comment

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

One comment on a comment (lol) but otherwise logic LGTM.

x/mongo/driver/operation.go Outdated Show resolved Hide resolved
Co-authored-by: Benjamin Rewis <32186188+benjirewis@users.noreply.github.com>
@prestonvasquez prestonvasquez merged commit 13ffe8d into mongodb:master Dec 8, 2022
@prestonvasquez prestonvasquez deleted the GODRIVER-2651 branch December 8, 2022 18:03
prestonvasquez added a commit to prestonvasquez/mongo-go-driver that referenced this pull request Dec 8, 2022
Co-authored-by: Qingyang Hu <103950869+qingyang-hu@users.noreply.github.com>
Co-authored-by: Benjamin Rewis <32186188+benjirewis@users.noreply.github.com>
Co-authored-by: Kevin Albertson <kevin.albertson@mongodb.com>
Co-authored-by: Qingyang Hu <103950869+qingyang-hu@users.noreply.github.com>
Co-authored-by: Benjamin Rewis <32186188+benjirewis@users.noreply.github.com>
prestonvasquez added a commit that referenced this pull request Dec 8, 2022
* GODRIVER-2651 Break NoWritesPerformed-Only Error Sequence (#1135)

Co-authored-by: Qingyang Hu <103950869+qingyang-hu@users.noreply.github.com>
Co-authored-by: Benjamin Rewis <32186188+benjirewis@users.noreply.github.com>
Co-authored-by: Kevin Albertson <kevin.albertson@mongodb.com>
Co-authored-by: Qingyang Hu <103950869+qingyang-hu@users.noreply.github.com>
Co-authored-by: Benjamin Rewis <32186188+benjirewis@users.noreply.github.com>

* GODRIVER-2333 Assert that Ping op succeeds  initial DNS spec tests (#1124)

* GODRIVER-2577 Retry heartbeat on timeout to prevent pool cleanup in FAAS pause. (#1133)

* resolve merge conflicts

Co-authored-by: Qingyang Hu <103950869+qingyang-hu@users.noreply.github.com>
Co-authored-by: Benjamin Rewis <32186188+benjirewis@users.noreply.github.com>
Co-authored-by: Kevin Albertson <kevin.albertson@mongodb.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants