-
Notifications
You must be signed in to change notification settings - Fork 246
DRIVERS-1819 Snapshot session sends readConcern with all commands even writes #1033
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
Conversation
eda705c to
11455ea
Compare
11455ea to
884cc09
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the approach. I implemented the changes in the Java driver, though, and some of the test fail on sharded 5.0 clusters. For example "Server returns an error on insertOne with snapshot" fails because the server does not throw an exception. I confirmed in the shell:
MongoDB Enterprise mongos> db.runCommand({'insert': 'test', documents: [{}], readConcern: {level: 'snapshot'}})
{
"n" : 1,
"ok" : 1,
"$clusterTime" : {
"clusterTime" : Timestamp(1625064185, 7),
"signature" : {
"hash" : BinData(0,"AAAAAAAAAAAAAAAAAAAAAAAAAAA="),
"keyId" : NumberLong(0)
}
},
"operationTime" : Timestamp(1625064185, 7)
}
Not sure what we should do. I suppose we could disable these tests on sharded clusters and report this as a server bug.
Update: I just noticed that 4 of the 9 tests in snapshot-sessions-unsupported-ops pass (findOneAndUpdate, listDatabase, listCollections, listIndexes), so the server is not consistent with this enforcement.
source/sessions/tests/unified/snapshot-sessions-unsupported-ops.json
Outdated
Show resolved
Hide resolved
|
Ah that's 100% correct. I had forgot to check the sharded test results. I also see mongos not validating readConcern on insert, update, and delete commands. I just opened SERVER-58176. |
|
Cool. Can you re-arrange the tests so that they are skipped on sharded clusters for now? |
Done. |
source/sessions/tests/unified/snapshot-sessions-unsupported-ops.json
Outdated
Show resolved
Hide resolved
|
@ajdavis Does this approach look good to you? For more background see the description in SERVER-58176. |
jyemin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This required a change in approach for the implementation, but it wasn't too big a deal in the end. All the tests are passing.
LGTM
| ================================================= | ||
|
|
||
| Drivers MUST set the readConcern ``level`` and ``atClusterTime`` fields (as | ||
| outlined above) on all commands in a snapshot session. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd add, "even commands like insert and update that do not accept a readConcern. This ensures the server will return an error for invalid operations, such as writes, within a session configured for snapshot reads." Make the implicit explicit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
|
Yeah, if we can fix SERVER-58176 in time, then I like this approach a lot. |
ShaneHarvey
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the quick reviews!
| ================================================= | ||
|
|
||
| Drivers MUST set the readConcern ``level`` and ``atClusterTime`` fields (as | ||
| outlined above) on all commands in a snapshot session. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
rstam
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. LGTM
* upstream/master: DRIVERS-1836 Skip tests with $out stage on serverless (mongodb#1036) DRIVERS-1816 Test redaction of replies to security sensitive commands (mongodb#1026) DRIVERS-1828: Remove insertedCount and improve use of $$unsetOrMatches (mongodb#1034) DRIVERS-1819 Disable writes and other unsupported operations in snapshot reads (mongodb#1033)
…hot reads (mongodb#1033) Rely on the server to report an error for unsupported snapshot read operations by sending readConcern with all commands, even writes.
This PR requires drivers to send readConcern with all commands in a snapshot session (even writes). This effectively bans all unsupported operations in a snapshot session because the server will reject commands that do not support readConcern.level "snapshot". For example:
I've added tests for a variety of methods to ensure we send readConcern.level correctly and that the server rejects the field. I can add tests for more operations once we decide this approach is reasonable.