DRIVERS-2782: Expose snapshotTime #1843
Conversation
source/sessions/snapshot-sessions.md
Outdated
| 1. If `snapshotTime` is specified in `SessionOptions`, saving the value in a `snapshotTime` property of the | ||
| `ClientSession`. | ||
| 2. If `snapshotTime` is not specified in `SessionOptions`, saving the `atClusterTime` returned by 5.0+ servers for the | ||
| first find/aggregate/distinct operation in a private `snapshotTime` property of the `ClientSession` object. Drivers |
There was a problem hiding this comment.
It's not a private property anymore, right?
source/sessions/snapshot-sessions.md
Outdated
|
|
||
| In order to support snapshot reads a new property named `snapshot` is added to `SessionOptions`. Applications set | ||
| `snapshot` when starting a client session to indicate whether they want snapshot reads. All read operations performed | ||
| In order to support snapshot reads two new properties called `snapshot` and `snapshotTime` are added to |
There was a problem hiding this comment.
I would avoid term "new", as that properties are new just for a little bit, but we are not going to change the spec just to mark them as "not new" 😊 .
There was a problem hiding this comment.
I agree with that 😁
I was trying to be consistent with the previous phrasing, but I'll fix it.
| atClusterTime: | ||
| "$$exists": true | ||
|
|
||
| ## This test seems to be wrong |
source/sessions/snapshot-sessions.md
Outdated
|
|
||
| Client MUST throw an error if `snapshotTime` is set and `snapshot` is not set to true. | ||
|
|
||
| Note that when parsing `snapshotTime` from `sessionOptions` for |
There was a problem hiding this comment.
Is this a good place to describe this behaviour?
There was a problem hiding this comment.
I would say we have to mention this in tests/README.md file.
papafe
changed the title
source/sessions/tests/README.md
Outdated
| Snapshot sessions tests require server of version 5.0 or higher and replica set or a sharded cluster deployment. | ||
|
|
||
| - `client.startSession(snapshot = false)` | ||
| - `client.snapshotTime` |
There was a problem hiding this comment.
it suppose to be session, right?
source/sessions/tests/README.md
Outdated
|
|
||
| Snapshot sessions tests require server of version 5.0 or higher and replica set or a sharded cluster deployment. | ||
|
|
||
| - `client.startSession(snapshot = false)` |
There was a problem hiding this comment.
I would prefer to have more verbal steps:
- Start a session by calling `startSession` with `snapshot = false`.
- Try to access the session's snapshot time.
- Assert that client side error was raised.
source/sessions/snapshot-sessions.md
Outdated
|
|
||
| Client MUST throw an error if `snapshotTime` is set and `snapshot` is not set to true. | ||
|
|
||
| Note that when parsing `snapshotTime` from `sessionOptions` for |
There was a problem hiding this comment.
I would say we have to mention this in tests/README.md file.
source/sessions/snapshot-sessions.md
Outdated
| Transactions are not allowed with snapshot sessions. Calling `session.startTransaction(options)` on a snapshot session | ||
| MUST raise an error. | ||
|
|
||
| Note that a new operation on session called `getSnapshotTime` must be supported for |
There was a problem hiding this comment.
Same here, move top tests/README.md
| expectResult: | ||
| - { _id: 1, x: 11 } | ||
| - { _id: 2, x: 11 } | ||
| - name: find |
There was a problem hiding this comment.
Why do we making find with the session2 twice, or have I missed some differences in the operations? If this is because we want to make sure snapshot time will not be changes after the first call - then it might makes sense to add comments here (same with other tests below).
There was a problem hiding this comment.
Yes, that's exactly the reason. Otherwise we'll have to resolve to creating more prose tests.
I'll add comments back in all the places where I'm doing this.
| expectEvents: | ||
| - client: client0 | ||
| events: | ||
| - commandStartedEvent: |
There was a problem hiding this comment.
Why don't we expect event for insertOne operation (here and in other tests).
There was a problem hiding this comment.
They are ignored in ignoreCommandMonitoringEvents inside createEntities for the client used in this whole test file. I suppose those events were ignored as they were not the focus of these tests.
C# driver implementation POC: mongodb/mongo-csharp-driver#1777
TODO:
clusters).
getSnapshotTimeoperation (for unified tests).snapshotTimeparameter ofsessionOptionsand that it must be retrieved from the entity map (for unified tests).