Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Comments

Close side panel

Updated GetSnapshot method#484

Closed
akg179 wants to merge 1 commit intoNEventStore:developNEventStore/NEventStore:developfrom
akg179:masterakg179/NEventStore:masterCopy head branch name to clipboard
Closed

Updated GetSnapshot method#484
akg179 wants to merge 1 commit intoNEventStore:developNEventStore/NEventStore:developfrom
akg179:masterakg179/NEventStore:masterCopy head branch name to clipboard

Conversation

@akg179
Copy link

@akg179 akg179 commented Jun 3, 2020

Currently, if two or more snapshots are added for a stream with same streamRevision using AddSnapshot method, both of them will be stored in the _snapshots collection. While retrieving snapshot for that streamRevision, the one which was added earlier in time will be returned from GetSnapshot.
I follow the MongoDB persistence engine and observe that AddSnapshot does an update instead of persisting all snapshots with same streamId and streamRevision.
Therefore, the InMemory persistence behaves differently when compared to MongoDB persistence.

With this change, the GetSnapshot has been modified to give the most recent added snapshot when there are more than one snapshots for same streamId and streamRevision.

Currently, if two or more snapshots are added for a stream with same streamRevision using AddSnapshot method, both of them will be stored in the `_snapshots` collection. While retrieving snapshot for that streamRevision, the one which was added earlier in time will be returned from `GetSnapshot`.
I follow the MongoDB persistence engine and observe that `AddSnapshot` does an `update` instead of persisting all snapshots with same `streamId` and `streamRevision`. https://github.com/NEventStore/NEventStore.Persistence.MongoDB/blob/master/src/NEventStore.Persistence.MongoDB/MongoPersistenceEngine.cs#L515 
Therefore, the InMemory persistence behaves differently when compared to MongoDB persistence.

With this change, the GetSnapshot has been modified to give the most recent added snapshot when there are more than one snapshots for same streamId and streamRevision.
@akg179 akg179 changed the base branch from master to develop June 3, 2020 17:35
@AGiorgetti
Copy link
Member

Hi, adding multiple snapshots for the same tuple of bucketId, streamId, streamRevision should not be allowed.
If the InMemoryPersistence allows to do that it is a bug.

I'll write a test to confirm the bug and fix the behavior.

@AGiorgetti
Copy link
Member

I double checked the behavior, actually it's up to the driver implementation decide what to do when adding more than one snapshot with the same tuple.
The SQL implementation ignores the second snapshot (and returns false) from the AddSnapshot method.
The MongoDB implementation updates the snapshot and return true.
At this point (without changing the original design) no assumption can be made other than this: if the AddSnapshot method returns true the "updated" snapshot should be returned, otherwise the original snapshot should be available.

@AGiorgetti
Copy link
Member

I have decided to not break the original behavior of the InMemoryPersistence and follow the Sql implementation for this provider.

I also added a test that checks the correct behavior of the provider:
If the AddSnapshot method returns true the most recent (and updated) snapshot will be returned, if it returns false the original one will be returned.

Thanks for reporting the Issue, I'm closing this PR.

@AGiorgetti AGiorgetti closed this Dec 3, 2020
AGiorgetti added a commit that referenced this pull request Dec 3, 2020
Also update the netcoreapp2.1 to netcoreapp3.1 for test projects.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.