CDRIVER-4110: Default for causal_consistency depends on snapshot #838
Conversation
…ot=true This was originally added in d5f73e1
This prose test is defined in the sessions spec, not transactions. It also does not require the server to support transactions.
Use mongoc_optional_t for causal_consistency and snapshot session opts to track whether or not values have been set. This also removes explicitly setting causal_consistency to true in _mongoc_client_session_new, since mongoc_session_opts_get_causal_consistency now enforces the default value.
| struct _mongoc_session_opt_t { | ||
| mongoc_session_flag_t flags; | ||
| mongoc_optional_t causal_consistency; | ||
| mongoc_optional_t snapshot; |
There was a problem hiding this comment.
Seems better to have these as optionals instead of on the same flag, thanks. Should the type be mongoc_optional_bool_t or is that type name change API-breaking?
There was a problem hiding this comment.
I brought that up with @kevinAlbs in Slack last week and we concluded that the ship has sailed since mongoc_optional_t is already in the public API (shipped with Versioned API in 1.18.0). If we end up introducing an optional int type down the line, perhaps we can deprecate the original name and introduce mongoc_optional_bool_t alongside mongoc_optional_int_t, but I don't think there's anything to do presently.
There was a problem hiding this comment.
Good catch @jmikola!
Here is my understanding.
Before snapshot reads:
mongoc_session_opts_newsetscausalConsistency=true.
After snapshot reads:
mongoc_session_opts_newsetscausalConsistency=trueandsnapshot=false.- Validation occurs in
mongoc_client_start_session(i.e. both being true is an error).
The new behavior proposed in this PR:
mongoc_session_opts_newsetscausalConsistency=unsetandsnapshot=unset- Defaults are applied in
mongoc_client_start_sessionif the values are unset. Validation occurs inmongoc_client_start_session.
Admittedly, when reviewing the snapshot reads changes, I had thought the behavior was correct since it built off of the previous behavior. I had not considered introducing an unset value for causalConsistency and snapshot. This seems correct.
There was a problem hiding this comment.
Your understanding is correct.
I think much of the confusion actually relates in some way to SPEC-936, which added the technical equivalent of legalese to state that the causalConsistency session option doesn't get a default value (which would require modeling it as a trilean), but instead inherits some default. In practice, I think some drivers (libmongoc included) just defaulted it to true as that had a functionally equivalent effect.
| @@ -47,6 +47,31 @@ test_session_opts_clone (void) | |||
| } | |||
|
|
|||
|
|
|||
| static void | |||
| test_session_opts_causal_consistency_and_snapshot (void) | |||
There was a problem hiding this comment.
Thanks for adding this test, we now have a similar one in the Go driver.
| struct _mongoc_session_opt_t { | ||
| mongoc_session_flag_t flags; | ||
| mongoc_optional_t causal_consistency; | ||
| mongoc_optional_t snapshot; |
There was a problem hiding this comment.
Good catch @jmikola!
Here is my understanding.
Before snapshot reads:
mongoc_session_opts_newsetscausalConsistency=true.
After snapshot reads:
mongoc_session_opts_newsetscausalConsistency=trueandsnapshot=false.- Validation occurs in
mongoc_client_start_session(i.e. both being true is an error).
The new behavior proposed in this PR:
mongoc_session_opts_newsetscausalConsistency=unsetandsnapshot=unset- Defaults are applied in
mongoc_client_start_sessionif the values are unset. Validation occurs inmongoc_client_start_session.
Admittedly, when reviewing the snapshot reads changes, I had thought the behavior was correct since it built off of the previous behavior. I had not considered introducing an unset value for causalConsistency and snapshot. This seems correct.
| @@ -2707,12 +2732,44 @@ test_session_dirty (void *unused) | |||
| _test_session_dirty_helper (false /* retry succceeds */); | |||
| } | |||
|
|
|||
| void | |||
| test_sessions_snapshot_prose_test_1 (void *ctx) | |||
There was a problem hiding this comment.
Thank you for moving this test.
https://jira.mongodb.org/browse/CDRIVER-4110
mongoc_optional_tinmongoc_session_opt_tto track values forcausalConsistencyandsnapshotoptions, and enforce thecausalConsistencylogic in the corresponding getter method.