Resolve snapshot check skipped for some events (#30225)

[jneumaier]

See #30225

Snapshotting check is omitted for most events and only checked for the last event in each batch of updates.

I added some local debug logging to RememberEntitiesShardStoreSpec test case "store starts and stops and list remembered entity ids" to show the behaviour:

Without the fix:
Recovery completed for shard [FakeShardId] with [0] entities
Persisted event, sequence number [1]
Checking for snapshot, sequence number [1], snapshot-after 2
Persisted event, sequence number [2]
Persisted event, sequence number [3]
Checking for snapshot, sequence number [3], snapshot-after 2
Persisted event, sequence number [4]
Checking for snapshot, sequence number [4], snapshot-after 2
Saving snapshot, sequence number [4]
Persisted event, sequence number [5]
Checking for snapshot, sequence number [5], snapshot-after 2

With the fix another check for snapshotting during the batch adds another snapshot:
Recovery completed for shard [FakeShardId] with [0] entities
Persisted event, sequence number [1]
Checking for snapshot, sequence number [1], snapshot-after 2
Persisted event, sequence number [2]
Checking for snapshot, sequence number [2], snapshot-after 2
Saving snapshot, sequence number [2]
Persisted event, sequence number [3]
Checking for snapshot, sequence number [3], snapshot-after 2
Persisted event, sequence number [4]
Checking for snapshot, sequence number [4], snapshot-after 2
Saving snapshot, sequence number [4]
Persisted event, sequence number [5]

[jneumaier]

@johanandren @octonato Please have a look at this fix.

[akka-ci]

Thank you for your pull request! After a quick sanity check one of the team will reply with 'OK ΤO ΤESΤ' to kick off our automated validation on Jenkins. This compiles the project, runs the tests, and checks for things like binary compatibility and source code formatting. When two team members have also manually reviewed and (perhaps after asking for some amendments) accepted your contribution, it should be good to be merged.

For more details about our contributing process, check out CONTRIBUTING.md - and feel free to ask!

[jneumaier]

On second thought I wonder if the state is always ready for snapshotting because it is only updated at the end of the batch. I might have to update the PR.

[patriknw]

Good catch

[patriknw]

I think we should still only save the snapshot after the last event, because otherwise the snapshot represents something partial.

I'd suggest another var that is updated for each event and then for the last event used as the condition for saveSnapshotWhenNeeded.

var saveSnap = false
...
... {
  saveSnap = saveSnap || isSnapshotNeeded
  if (isLastEvent && saveSnap)
    saveSnapshot()
}

[jneumaier]

Good idea. I thought the snapshot should be saved on the exact index but it would not represent the correct sequence number anyway including future events of the batch.

I think it is more clear and logically correct now.

[jneumaier]

I thought once more why I first suggested a snapshot on the exact sequence number divided by snapshotAfter:

https://github.com/akka/akka/blob/73d74dfc69b26cc8dea5a6800ec6ad6cd28168f9/akka-cluster-sharding/src/main/scala/akka/cluster/sharding/internal/EventSourcedRememberEntitiesShardStore.scala#L129

We will break this logic with the suggestion. What do you think @patriknw ?

[johanandren]

Can't we always subtract a full batch size from the calculated fromOffset, that way it doesn't matter where in the batch the offset happened?

[jneumaier]

@johanandren Do you mean just keep one additional batch for this special circumstance? I would be fine with that.

[johanandren]

Yes, that is the idea I had.

[johanandren]

OK TO TEST

[johanandren]

Another good catch, I think the logic looks good, but that the deletion needs an update to not "leak" events when deleting.

[akka-ci]

Test PASSed.

[patriknw]

@jneumaier There is now a bin compatibility warning. Could you adjust the mima filter?

Did we resolve the previous discussion about when to snapshot or is there something more?
#30226 (comment)

[jneumaier]

@jneumaier There is now a bin compatibility warning. Could you adjust the mima filter?

Did we resolve the previous discussion about when to snapshot or is there something more?
#30226 (comment)

@patriknw
Yes there is one more update to do to keep one additional batch of journal messages. I will also update the mima filter. I will push the changes tomorrow.

[patriknw]

I will push the changes tomorrow.

Excellent, we are planning a release on Friday and this would be great to have included. Thanks for excellent work.

[akka-ci]

Test PASSed.

[akka-ci]

Test PASSed.

[jneumaier]

All checks green, ready for final review.

[jneumaier]

Only improved reference.conf documentation with last pushed commit.

[akka-ci]

Test PASSed.

[patriknw]

LGTM, thanks

[johanandren]

LGTM!