Skip to content

Conversation

@prestonvasquez
Copy link
Member

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.

@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
Member Author

@prestonvasquez prestonvasquez Dec 6, 2022

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.

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!

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.

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.

@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 <[email protected]>
Co-authored-by: Benjamin Rewis <[email protected]>
Co-authored-by: Kevin Albertson <[email protected]>
Co-authored-by: Qingyang Hu <[email protected]>
Co-authored-by: Benjamin Rewis <[email protected]>
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 <[email protected]>
Co-authored-by: Benjamin Rewis <[email protected]>
Co-authored-by: Kevin Albertson <[email protected]>
Co-authored-by: Qingyang Hu <[email protected]>
Co-authored-by: Benjamin Rewis <[email protected]>

* 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 <[email protected]>
Co-authored-by: Benjamin Rewis <[email protected]>
Co-authored-by: Kevin Albertson <[email protected]>
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.

5 participants