fix(NODE-3541): allow unacknowledged write with hint #3415
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
5 tasks
|
Hey @LinusU - thanks for this contribution. There are still some open questions about this ticket and at this time, it's not ready to be implemented. Our team will need to discuss the ticket in refinement to iron out some of the details. We don't have the bandwidth to pick this up at the moment but we will put the ticket in our triage queue to revisit it when we get the chance and when we do implement it, we'll make sure you get credit for your contribution as well. I think we can move forward with #3409 in the meantime though! |
|
I understand, let me know if there is anything you want me to do here 👍 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This is a replacement to #3409, from the knowledge I got there I could implement this properly instead of just improving the error message!
Now it actually fixes the entire ticket 🎉
What is changing?
The driver now allows unacknowledged writes when a hint is present in the following scenarios:
updatecalls when the server is 4.2 or newerdeletecalls when the server is 4.4 or newerIs there new documentation needed for these changes?
Not as far as I know, because I couldn't find any documentation on this limit in the first place.
What is the motivation for this change?
This allows us to submit a hint to an unacknowledged write. See more background in NODE-3541.
Double check the following
npm run check:lintscript<type>(NODE-xxxx)<!>: <description>I also need to update the tests here, but since this is my first contribution here I would love some guidance.
What I think we need to do is to update the
test/spec/crud/unified/unacknowledged-*-hint-clientError.ymlfiles and make them only test for the error on specific server versions, but expect success on others. If anyone could point me in the right direction here I would be grateful. Should we also rename the files to remove "clientError" from the name? Actually, the ticket mentions syncing unified CRUD tests, that might be what we should instead?ping @baileympearson, thank you for reviewing my first PR, and giving me info to continue this!