CDRIVER-4034 Allow snapshot reads through sessions #807
Conversation
There was a problem hiding this comment.
This is looking like a great start!
I have not done a line-by-line review, but I have high level questions about:
- Can we hide the API for setting
atClusterTimeon a read concern? - Can we handle
atClusterTimesimilarly toafterClusterTimeto simplify how the read concern is appended?
There was a problem hiding this comment.
Refactored slightly to append snapshot to read_concern in _mongoc_client_session_append_read_concern. This seems simpler and more accurate to me; thanks for the suggestion @kevinAlbs.
Implemented the one prose test, too (CDRIVER-4037). I had mentioned offline there may be Evergreen changes for the test to set minSnapshotHistoryWindowInSeconds on the server, but I'm unsure if this is strictly necessary.
There was a problem hiding this comment.
Looks like the prose test no longer requires that we set minSnapshotHistoryWindowInSeconds, so it should be all set to review.
| MONGOC_ERROR_TRANSACTION_INVALID_STATE, | ||
| "Transactions are not supported in snapshot sessions"); | ||
| ret = false; | ||
| GOTO (done); |
There was a problem hiding this comment.
This new error is an alteration to the spec accompanied by a new spec test "StartTransaction fails in snapshot session".
There was a problem hiding this comment.
Fantastic work. LGTM with a minor test tweak. Let's wait until https://github.com/mongodb/specifications/pull/1022/files/dfb2472e96e1e3f4f374eb526c5190c8282239b0..e1e0672414ff7756de2a2fb9a5d4b7ddea7c1066 is merged to merge this in case there are any spec / test changes to add.
| is_snapshot = | ||
| is_read_command && mongoc_session_opts_get_snapshot (&cs->opts); | ||
| is_snapshot = is_find_aggregate_distinct && | ||
| txn_state == MONGOC_INTERNAL_TRANSACTION_NONE && |
There was a problem hiding this comment.
No need to change. But IIUC, this transaction check should be unnecessary. It is impossible to start a transaction on a session with snapshot enabled.
There was a problem hiding this comment.
Yep! Now that we explicitly error in mongoc_client_session_start_transaction when snapshot is enabled on the session, I believe this check is pointless. Removed.
CDRIVER-4034
Adds
MONGOC_SESSION_SNAPSHOTflag option tomongoc_session_opt_t. Setting this flag will mean:atClusterTimein the reply cursoratClusterTimewill be stored as new fieldssnapshot_time_timestampandsnapshot_time_incrementon themongoc_client_session_tatClusterTimeset to thesnapshot_timestored on the client session.These changes will allow users to read from a single point in time (a snapshot) during a session.
Adds new
at_cluster_time_timestampandat_cluster_time_incrementfields to read concern (along with getters) and a newmongoc_read_concern_set_at_cluster_timesetter. Allows settingMONGOC_SESSION_SNAPSHOTwithisSnapshotin the unified test runner. Syncs sessions spec tests.