RUST-48 Causal consistency support #493
Conversation
patrickfreed
force-pushed
the
RUST-48_Causal-Consistency
branch
from
9b721da to
da602c1
Compare
src/client/options/mod.rs
Outdated
| @@ -466,6 +466,7 @@ pub struct ClientOptions { | |||
| /// Specifies the default read concern for operations performed on the Client. See the | |||
| /// ReadConcern type documentation for more details. | |||
| #[builder(default)] | |||
| #[serde(skip_serializing)] | |||
There was a problem hiding this comment.
Why does this need to be skipped?
There was a problem hiding this comment.
Read concern is now included in the Command struct, so it needs to be skipped here otherwise it'll be serialized twice.
There was a problem hiding this comment.
That makes sense for the individual command options updated elsewhere, but I don't think ClientOptions is included in the serialized command form? AFAICT ClientOptions only implements Serialize / Deserialize for test code.
There was a problem hiding this comment.
Ohh I see, this was just a search/replace mistake. Fixed.
| s, | ||
| ))); | ||
|
|
||
| ops |
There was a problem hiding this comment.
Nit: can remove a little bit of noise here by preemptively returning ops.into_iter() and updating the signature to return impl Iterator.
src/cmap/conn/command.rs
Outdated
| at_cluster_time: None, | ||
| after_cluster_time: None, | ||
| }); | ||
| inner.level = Some(read_concern.level.clone()); |
There was a problem hiding this comment.
Since this now only sets the level, it seems like it'd be clearer to have it only accept the level as a parameter.
src/operation/mod.rs
Outdated
| @@ -124,6 +127,12 @@ pub(crate) trait Operation { | |||
| None | |||
| } | |||
|
|
|||
| /// Returns whether or not this command supports the `readConcern` field, and if so, the read | |||
There was a problem hiding this comment.
Comment doesn't seem accurate?
src/client/options/mod.rs
Outdated
| /// | ||
| /// Defaults to true. | ||
| pub causal_consistency: Option<bool>, | ||
|
|
||
| /// If true, all read operations performed using this client session will share the same | ||
| /// snapshot. Defaults to false. | ||
| // TODO RUST-18 enforce snapshot exclusivity with causalConsistency. |
There was a problem hiding this comment.
I think we can delete this TODO (looks like it's pointing to the wrong ticket)
src/client/options/mod.rs
Outdated
| /// If true, all operations performed in the context of this session | ||
| /// will be [causally consistent](https://docs.mongodb.com/manual/core/causal-consistency-read-write-concerns/). | ||
| /// | ||
| /// Defaults to true. |
There was a problem hiding this comment.
Can we clarify this to indicate that this field only defaults to true if snapshot is unspecified?
| @@ -91,6 +92,12 @@ impl Operation for Aggregate { | |||
| .and_then(|opts| opts.selection_criteria.as_ref()) | |||
| } | |||
|
|
|||
| fn supports_read_concern(&self, description: &StreamDescription) -> bool { | |||
There was a problem hiding this comment.
Does this have to do with causal consistency or is this a validation we were missing?
There was a problem hiding this comment.
Causal consistency requires that we set a read concern in the executor now, whereas before individual operations were in charge of serializing read concerns. Now that the executor is in charge of that, it needs to know when it can set them, hence this method. The check for it can be seen here: https://github.com/awitten1/mongo-rust-driver/blob/RUST-48_Causal-Consistency/src/client/executor.rs#L430
I think users setting read concerns on operations that don't support them is one of those things we're meant to leave to the server to validate, but we do need to validate that the driver doesn't accidentally set a read concern behind the scenes when it shouldn't.
This PR provides support for causal consistency.
The change that is needed to support causally consistent sessions is that
readConcern.afterClusterTimenow needs to be included in all commands that are part of a causally consistent session that take areadConcern.readConcern.afterClusterTimemust be set to theoperationTimefield of theClientSessionstruct (initialized toNone). TheoperationTimefield of theClientSessionstruct must get updated to theoperationTimefield of the server's response to any command that is part of a causally consistent session (this field is included on success or error).The main difficulty is that
readConcern.levelnow is not always included whenreadConcernis included. To address that I introduced a new typereadConcernInternalthat takes the role ofreadConcernin certain cases (whenlevelis optional).Currently there is a bug in this implementation where there are duplicate
readConcernfields being included. Most likely this is because the readConcern inFindOptions(for example) and thereadConcernthat is set on the session are both being serialized.