Akka.Persistence: Made `DateTime.UtcNow` the default timestamp for `SnapshotMetdata` #7313

Aaronontheweb · 2024-08-06T16:12:54Z

Changes

Checklist

For significant changes, please ensure that the following have been completed (delete if not relevant):

This change follows the Akka.NET API Compatibility Guidelines.
This change follows the Akka.NET Wire Compatibility Guidelines.
I have reviewed my own pull request.
Design discussion issue Akka.Persistence: SnapshotMetadata always uses DateTime.MinValue, which creates "duplicate key" exceptions when multiple snapshots are taken at same seqNr #7312
Changes in public API reviewed, if any.

Aaronontheweb · 2024-08-06T16:13:27Z

Not done with this PR by a long shot - looking at doing some potential fixes around the ITimeProvider implementation we have first in order to ensure that we ACTUALLY support dependency injection with it.

Aaronontheweb

Detailed my changes

Aaronontheweb · 2024-08-06T17:33:09Z

src/core/Akka.API.Tests/verify/CoreAPISpec.ApprovePersistence.DotNet.verified.txt

@@ -587,7 +587,8 @@ namespace Akka.Persistence
    }
    public sealed class SnapshotMetadata : System.IEquatable<Akka.Persistence.SnapshotMetadata>
    {
-        public static System.DateTime TimestampNotSpecified;
+        [System.ObsoleteAttribute("This constructor is deprecated and will be removed in v1.6. Use the constructor w" +


Just marked the old CTOR as Obsolete - no other API changes.

Aaronontheweb · 2024-08-06T17:36:10Z

src/core/Akka.Persistence.TCK/Query/PersistenceIdsSpec.cs

@@ -247,7 +247,7 @@ protected IActorRef WriteSnapshot(string persistenceId, int n)
                ExpectMsg($"{persistenceId}-{i}-done");
            }

-            var metadata = new SnapshotMetadata(persistenceId, n + 10);
+            var metadata = new SnapshotMetadata(persistenceId, n + 10, Sys.Scheduler.Now.DateTime);


Standard call we're going to use to get the current UTC time from the ITimeProvider built into the IScheduler.

Aaronontheweb · 2024-08-06T17:36:52Z

src/core/Akka.Persistence/Eventsourced.cs

@@ -218,7 +218,7 @@ public void LoadSnapshot(string persistenceId, SnapshotSelectionCriteria criteri
        /// <param name="snapshot">TBD</param>
        public void SaveSnapshot(object snapshot)
        {
-            SnapshotStore.Tell(new SaveSnapshot(new SnapshotMetadata(SnapshotterId, SnapshotSequenceNr), snapshot));
+            SnapshotStore.Tell(new SaveSnapshot(new SnapshotMetadata(SnapshotterId, SnapshotSequenceNr, Context.System.Scheduler.Now.Date), snapshot));


Using the same standard call now for both saving and deleting snapshots - big question I have though: does this break deleting snapshots? It SHOULDN'T, but we'll need to see how the specs perform to know for certain.

Arkatufus

After thinking over about it, I think the change is dangerous

Arkatufus · 2024-08-07T15:28:38Z

src/core/Akka.API.Tests/verify/CoreAPISpec.ApprovePersistence.Net.verified.txt

@@ -587,7 +587,8 @@ namespace Akka.Persistence
    }
    public sealed class SnapshotMetadata : System.IEquatable<Akka.Persistence.SnapshotMetadata>
    {
-        public static System.DateTime TimestampNotSpecified;
+        [System.ObsoleteAttribute("This constructor is deprecated and will be removed in v1.6. Use the constructor w" +


A little nitpick, Would be better if we follow english language sentence breaking point here, makes it easier to read.

Good point - I can fix that.

Ah, no I can't - that's an artifact from the rendering of the API diff. The real code looks like this:

[Obsolete("This constructor is deprecated and will be removed in v1.6. Use the constructor with the timestamp parameter instead.", true)]

src/core/Akka.Persistence/SnapshotProtocol.cs

Arkatufus

Self review

Arkatufus · 2024-08-08T19:41:10Z

src/core/Akka.Persistence.TCK/Snapshot/SnapshotStoreSpec.cs

+        // Issue #7312, this is a side effect of the ctor signature changes
+        // DeleteSnapshot should not delete snapshot if timestamp value does not meet deletion criteria
+        [Fact]
+        public virtual void SnapshotStore_should_not_delete_snapshot_identified_by_SequenceNr_if_metadata_timestamp_is_less_than_stored_timestamp()


This is a side effect of the changes made in this PR.
User need to either have a SnapshotMetadata.Timestamp that is greater than or equals to the stored value, or equals to DateTime.MinValue for the actual Snapshot to be deleted.

We either have this as an actual spec, or change the code to disregard SnapshotMetadata.Timestamp altogether when deleting a Snapshot.

Should we add an overrideable virtual variable that allows this spec to be enable / disable on an implementation by implementation basis?

The unit test methods themselves are virtual, and this is part of the spec now.
I think it is reasonable to force the other SQL implementation to adhere to the new spec?

Well, "force", someone can always override and skip the test in implementation

Doh, I missed that. You're right. LGTM.

Aaronontheweb

Left some comments

Aaronontheweb · 2024-08-08T20:12:50Z

src/contrib/persistence/Akka.Persistence.Sql.Common/Snapshot/QueryExecutor.cs

@@ -463,7 +463,7 @@ protected virtual void SetManifestParameters(object snapshot, DbCommand command)
            await connection.ExecuteInTransaction(WriteIsolationLevel, cancellationToken, async (tx, token) =>
            {
                var sql = timestamp.HasValue
-                    ? DeleteSnapshotRangeSql + " AND { Configuration.TimestampColumnName} = @Timestamp"
+                    ? DeleteSnapshotSql + $" AND {Configuration.TimestampColumnName} <= @Timestamp"


LGTM - bugfix

Aaronontheweb · 2024-08-08T20:13:28Z

src/core/Akka.Persistence.TCK/Snapshot/SnapshotStoreSpec.cs

+        // Issue #7312, this is a side effect of the ctor signature changes
+        // DeleteSnapshot should not delete snapshot if timestamp value does not meet deletion criteria
+        [Fact]
+        public virtual void SnapshotStore_should_not_delete_snapshot_identified_by_SequenceNr_if_metadata_timestamp_is_less_than_stored_timestamp()


Should we add an overrideable virtual variable that allows this spec to be enable / disable on an implementation by implementation basis?

Left-over bugs from akkadotnet#7313

* fixed Akka.Persistence.Tck snapshot load specs Left-over bugs from #7313 * fixed the specs

…estamp for `SnapshotMetdata`

…estamp for `SnapshotMetdata` (cherry picked from commit 916d18b)

* Bump AkkaVersion from 1.5.26 to 1.5.28 Bumps `AkkaVersion` from 1.5.26 to 1.5.28. Updates `Akka.Cluster.Sharding` from 1.5.26 to 1.5.28 - [Release notes](https://github.com/akkadotnet/akka.net/releases) - [Changelog](https://github.com/akkadotnet/akka.net/blob/dev/RELEASE_NOTES.md) - [Commits](akkadotnet/akka.net@1.5.26...1.5.28) Updates `Akka.Persistence.Query` from 1.5.26 to 1.5.28 - [Release notes](https://github.com/akkadotnet/akka.net/releases) - [Changelog](https://github.com/akkadotnet/akka.net/blob/dev/RELEASE_NOTES.md) - [Commits](akkadotnet/akka.net@1.5.26...1.5.28) Updates `Akka.Persistence.TCK` from 1.5.26 to 1.5.28 - [Release notes](https://github.com/akkadotnet/akka.net/releases) - [Changelog](https://github.com/akkadotnet/akka.net/blob/dev/RELEASE_NOTES.md) - [Commits](akkadotnet/akka.net@1.5.26...1.5.28) --- updated-dependencies: - dependency-name: Akka.Cluster.Sharding dependency-type: direct:production update-type: version-update:semver-patch - dependency-name: Akka.Persistence.Query dependency-type: direct:production update-type: version-update:semver-patch - dependency-name: Akka.Persistence.TCK dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> * Port akkadotnet/akka.net#7313: Made `DateTime.UtcNow` the default timestamp for `SnapshotMetdata` (cherry picked from commit 916d18b) * Make operation cheaper (cherry picked from commit 1f8c3f6) * Fix port code to take DateTime.MinValue into account --------- Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Gregorius Soedharmo <[email protected]>

Made DateTime.UtcNow the default timestamp for SnapshotMetdata

333b5c2

Aaronontheweb added the akka-persistence label Aug 6, 2024

Aaronontheweb added 3 commits August 6, 2024 12:27

fixed all SnapshotMetadata calls

930414f

added API approvals

788111a

standardized on Sys.Scheduler.Now.DateTime

28b94b5

Aaronontheweb commented Aug 6, 2024

View reviewed changes

Aaronontheweb marked this pull request as ready for review August 6, 2024 17:39

Arkatufus requested changes Aug 7, 2024

View reviewed changes

Arkatufus and others added 3 commits August 8, 2024 03:32

Merge branch 'dev' into fix-7312-SnapshotMetadata

64b00ed

Merge branch 'dev' into fix-7312-SnapshotMetadata

736d3b8

Fix SQL query error in QueryExecutor

6851492

Arkatufus reviewed Aug 8, 2024

View reviewed changes

Arkatufus approved these changes Aug 8, 2024

View reviewed changes

Update API Approval list

85c143a

Aaronontheweb added the akka-persistence-sql-common Akka.Persistence.Sql.Common label Aug 8, 2024

Aaronontheweb commented Aug 8, 2024

View reviewed changes

Aaronontheweb merged commit d6a8063 into akkadotnet:dev Aug 8, 2024
7 of 12 checks passed

Aaronontheweb deleted the fix-7312-SnapshotMetadata branch August 8, 2024 20:23

Aaronontheweb added a commit to Aaronontheweb/akka.net that referenced this pull request Aug 16, 2024

fixed Akka.Persistence.Tck snapshot load specs

ae2ca42

Left-over bugs from akkadotnet#7313

Aaronontheweb mentioned this pull request Aug 16, 2024

fixed Akka.Persistence.Tck snapshot load specs #7320

Merged

4 tasks

Arkatufus pushed a commit that referenced this pull request Aug 16, 2024

fixed Akka.Persistence.Tck snapshot load specs (#7320)

96c6a60

* fixed Akka.Persistence.Tck snapshot load specs Left-over bugs from #7313 * fixed the specs

Arkatufus mentioned this pull request Aug 23, 2024

Update RELEASE_NOTES.md for 1.5.28-beta1 release #7329

Merged

Arkatufus added a commit to Arkatufus/Akka.Persistence.Sql that referenced this pull request Aug 30, 2024

Port akkadotnet/akka.net#7313

8975c0f

Arkatufus mentioned this pull request Aug 30, 2024

Port akkadotnet/akka.net#7313 akkadotnet/Akka.Persistence.Sql#448

Merged

Aaronontheweb pushed a commit to akkadotnet/Akka.Persistence.Sql that referenced this pull request Sep 4, 2024

Port akkadotnet/akka.net#7313 (#448)

64bdd34

Arkatufus mentioned this pull request Sep 4, 2024

Update RELEASE_NOTES.md for 1.5.28 release #7336

Merged

Arkatufus added a commit to Arkatufus/Akka.Persistence.Azure that referenced this pull request Sep 11, 2024

Port akkadotnet/akka.net#7313: Made DateTime.UtcNow the default tim…

916d18b

…estamp for `SnapshotMetdata`

Arkatufus added a commit to Arkatufus/Akka.Persistence.Azure that referenced this pull request Sep 11, 2024

Port akkadotnet/akka.net#7313: Made DateTime.UtcNow the default tim…

e755ab7

…estamp for `SnapshotMetdata` (cherry picked from commit 916d18b)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Akka.Persistence: Made `DateTime.UtcNow` the default timestamp for `SnapshotMetdata` #7313

Akka.Persistence: Made `DateTime.UtcNow` the default timestamp for `SnapshotMetdata` #7313

Aaronontheweb commented Aug 6, 2024 •

edited

Loading

Aaronontheweb commented Aug 6, 2024

Aaronontheweb left a comment

Aaronontheweb Aug 6, 2024

Aaronontheweb Aug 6, 2024

Aaronontheweb Aug 6, 2024

Arkatufus left a comment

Arkatufus Aug 7, 2024

Aaronontheweb Aug 8, 2024

Aaronontheweb Aug 8, 2024

Arkatufus left a comment

Arkatufus Aug 8, 2024 •

edited

Loading

Arkatufus Aug 8, 2024

Aaronontheweb Aug 8, 2024

Arkatufus Aug 8, 2024

Arkatufus Aug 8, 2024

Aaronontheweb Aug 8, 2024

Aaronontheweb left a comment

Aaronontheweb Aug 8, 2024

Aaronontheweb Aug 8, 2024

Akka.Persistence: Made DateTime.UtcNow the default timestamp for SnapshotMetdata #7313

Akka.Persistence: Made DateTime.UtcNow the default timestamp for SnapshotMetdata #7313

Conversation

Aaronontheweb commented Aug 6, 2024 • edited Loading

Changes

Checklist

Aaronontheweb commented Aug 6, 2024

Aaronontheweb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Arkatufus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Arkatufus left a comment

Choose a reason for hiding this comment

Arkatufus Aug 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Aaronontheweb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Akka.Persistence: Made `DateTime.UtcNow` the default timestamp for `SnapshotMetdata` #7313

Akka.Persistence: Made `DateTime.UtcNow` the default timestamp for `SnapshotMetdata` #7313

Aaronontheweb commented Aug 6, 2024 •

edited

Loading

Arkatufus Aug 8, 2024 •

edited

Loading