Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Snapshot resulting in inconsistent aggregate state when aggregate field or eventsourcing logic changed #566

Closed
shysank opened this issue Apr 7, 2018 · 6 comments
Assignees
Labels
Priority 2: Should High priority. Ideally, these issues are part of the release they’re assigned to. Status: Resolved Use to signal that work on this issue is done. Type: Feature Use to signal an issue is completely new to the project.
Milestone

Comments

@shysank
Copy link
Contributor

shysank commented Apr 7, 2018

When an aggregate structure is changed (eg. a field is added), the aggregate by concatenating snapshot and events may result in an inconsistent state. This is because the snapshot will have the old aggregate payload and will not have considered the new fields added after the snapshot was saved.

The same is true when the eventsourcing logic is changed (eg. previousy item was added to the bottom of the list, now it is changed to add to the top of the list). Again the snapshot would have the list in the normal order, but we now want it in the reverse order resulting in an inconsistent state.

Proposed Solution:

  1. Create a metadata entry in snapshot containing a hash of the aggregate type's useful content (fields + eventsourcing handlers)
  2. While reading the snapshot, compare the current hash with the snapshot's hash.
  3. If they don't match, discard the snapshot and replay events to build the aggregate. Delete the snapshot.

I'm thinking of something like:

interface AggregateTypeResolver{

default String resolve(Class aggregateType){
    return hash(aggregateType) //use reflection/language parsing
}

}

interface SnapshotAggregateTypeValidator{

default String hasChanged(DomainEventMessage snapshot){
    return snapshot.getMetaData().get("aggregateTypeHash")
            .equals(resolver.resolve(Class.forName(snapshot.getType())))
}

}
`

Happy to work on a pr if you want.

@abuijze
Copy link
Member

abuijze commented Apr 10, 2018

Hi Cynic,

this is indeed a potential issue. However, the extra line of defense you suggested doesn't really close the gap. For this to really work, the hash would need to be created based on the implementation of the method.

Something that sprung to mind is to use the RevisionResolver to detect a difference between the Aggregate class' revision, and that of the snapshot. If there is a difference, the snapshot should be disregarded.

Also, instead of deleting the snapshot, just ignore it and choose the next one. I have seen project that implemented concurrent versions in snapshots to allow them to prepare snapshots as part of a blue-green deployment. The old (still running) version would need to ignore snapshots that are created for the newer version, which being prepared to take over.

What do you think?

@shysank
Copy link
Contributor Author

shysank commented Apr 10, 2018

Hi Allard,

The RevisionResolver used for resolving event payloads seems like a good match for this scenario. It is possible to do a similar thing (@AggregateRevision) for Aggregate payloads, and it would be easier to compare and infer than a hash.

Having said that, making aggregate class version explicit also makes it optional. I think this feature should not be optional. Also, unlike event payloadtype, one has to also update the version when there is a logic change in eventsoucing handlers. This may feel counter intuitive because the structure of the payload has'nt changed.

For finding implementation changes, we could use tools like antlr to parse the ast. Worst case implementation would be to hash the entire file. It would take a hit in performance for sometime, but atleast the state would be correct.

As to deleting snapshot, I agree we should not do that and support multiple versions.

@ravindrabhatt
Copy link

@abuijze Is there a plan to fix this in any upcoming release of axon? We are facing the same issue.

@ravindrabhatt
Copy link

In axon 3 it actually works. The snapshot is treated just like any other event which, means it is up-casted as well. You can just add @revision annotation on aggregate and write upcasters just like any other event. I tried it in axon 3 and it works.

@smcvb smcvb added the Type: Feature Use to signal an issue is completely new to the project. label Apr 30, 2018
@smcvb smcvb added this to the Release 3.3 milestone Apr 30, 2018
@abuijze
Copy link
Member

abuijze commented Jun 25, 2018

The PR (#583) attached to this issue has been merged. Another issue (#614) has been created to build upon this feature, where multiple snapshots can concurrently live and an application version takes the most recent snapshot that matches its requirements.

@abuijze abuijze closed this as completed Jun 25, 2018
@shysank
Copy link
Contributor Author

shysank commented Jun 27, 2018

@abuijze thanks for this. Also want to remind you that I did not add the changes for other eventstorage engines apart from jdbc (jpa and mongo).

@smcvb smcvb added Priority 2: Should High priority. Ideally, these issues are part of the release they’re assigned to. Status: Resolved Use to signal that work on this issue is done. labels Feb 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority 2: Should High priority. Ideally, these issues are part of the release they’re assigned to. Status: Resolved Use to signal that work on this issue is done. Type: Feature Use to signal an issue is completely new to the project.
Projects
None yet
Development

No branches or pull requests

4 participants