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

DRIVERS-2695 Add a case of CSOT not enabled for the retryable writes. #1466

Merged
merged 2 commits into from
Oct 19, 2023

Conversation

qingyang-hu
Copy link
Contributor

  • Add the case the pseudo-code for the retryable writes specification: CSOT is not enabled and one retry was attempted.
  • Fix some syntax errors.

Please complete the following before merging:

  • Update changelog.
  • Make sure there are generated JSON files from the YAML test files.
  • Test changes in at least one language driver.
  • Test these changes against all server versions and topologies (including standalone, replica set, sharded clusters, and serverless).

Driver changes are not needed.

@qingyang-hu qingyang-hu requested a review from a team as a code owner October 4, 2023 14:25
@qingyang-hu qingyang-hu requested review from kkloberdanz and ShaneHarvey and removed request for a team October 4, 2023 14:25
Copy link
Contributor

@kkloberdanz kkloberdanz 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!

handleError(secondError);
throw secondError;
}
break
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't account for NoWritesPerformed or error code 20. Is it possible to avoid duplicating the call to executeCommand and other logic here? Perhaps:

if ((timeoutMS == null && retrying) || (timeoutMS != null && isExpired(timeoutMS))) {
    throw previousError;
}
retrying = True;

@@ -519,23 +520,15 @@ The above rules are implemented in the following pseudo-code:
if (timeoutMS == null) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have not merged the conditions to keep the reference code straightforward with the comments.

Copy link
Member

@ShaneHarvey ShaneHarvey left a comment

Choose a reason for hiding this comment

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

LGTM!

@qingyang-hu qingyang-hu merged commit d1157f7 into master Oct 19, 2023
3 checks passed
@qingyang-hu qingyang-hu deleted the drivers2695 branch October 19, 2023 19:50
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.

3 participants