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

Sort events before applying them to aggregate #411

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

totemcaf
Copy link

@totemcaf totemcaf commented Aug 1, 2023

Description

When an aggregate is materialized, events are read from the event source. In the case of the MongoDB implementation, the way they are read does not guarrants the event order.

This change sort events before applying them.

Affected Components

  • aggregatestore.events.AggregateStore

Related Issues

#409

Solution and Design

Events are sorted by version number before being applied. Because version number is an integer between A and B, being A the first version read, and B the last, the sort uses the version position to insert in an slice of size B-A+1.

Solution was benchmarked:

$go test -bench=. -run=^# -benchtime 10s -cpu 1 -benchmem
goos: linux
goarch: amd64
pkg: github.com/looplab/eventhorizon/aggregatestore/events
cpu: 11th Gen Intel(R) Core(TM) i7-1165G7 @ 2.80GHz
BenchmarkSort/sortEventsByVersion-1             218145657             55.39 ns/op           16 B/op          1 allocs/op
BenchmarkSort/sortEventsByVersion-10             53875314             216.9 ns/op          160 B/op          1 allocs/op
BenchmarkSort/sortEventsByVersion-100             7211017              1639 ns/op         1792 B/op          1 allocs/op
BenchmarkSort/sortEventsByVersion-1000             627288             18589 ns/op        16384 B/op          1 allocs/op
BenchmarkSortUnsorted
BenchmarkSortUnsorted/sortEventsByVersion-1     220128056             54.52 ns/op           16 B/op          1 allocs/op
BenchmarkSortUnsorted/sortEventsByVersion-10     54352377             219.1 ns/op          160 B/op          1 allocs/op
BenchmarkSortUnsorted/sortEventsByVersion-100     7374812              1629 ns/op         1792 B/op          1 allocs/op
BenchmarkSortUnsorted/sortEventsByVersion-1000     657199             18231 ns/op        16384 B/op          1 allocs/op
PASS
ok      github.com/looplab/eventhorizon/aggregatestore/events   110.407s

Steps to test and verify

aggregatestore_sort_test.go was added.

You can also configure an aggregate with a MongoDB-2 event store.

  1. Create an aggregate
  2. Execute a command in aggregate that generates one or more events
  3. Repeat 2 several times
  4. No errors should be detected

@coveralls
Copy link

coveralls commented Aug 1, 2023

Coverage Status

coverage: 67.175% (-0.2%) from 67.361%
when pulling b92591c on AltScore:fix/409-event-order-application
into ac3a972 on looplab:main.

@MaxBreida
Copy link
Contributor

Hey, why the decision to do this manual kind of sorting instead of using the sort package?

@totemcaf
Copy link
Author

Hey, why the decision to do this manual kind of sorting instead of using the sort package?

The idea is to take advantage of the event structure to improve performance because there was a concern about it.

@totemcaf
Copy link
Author

@MaxBreida

Today we suffer in tests from this bug, so I take a new look into it.

I reverted original solution and I provided now a new more modular one.

Basically the mongodb_v2 implementation of the event store will warrant the event order.

For other implementation, a EventSorter wrapper is provided that sort events got by Load and LoadFrom methods.

@MaxBreida
Copy link
Contributor

@MaxBreida

Today we suffer in tests from this bug, so I take a new look into it.

I reverted original solution and I provided now a new more modular one.

Basically the mongodb_v2 implementation of the event store will warrant the event order.

For other implementation, a EventSorter wrapper is provided that sort events got by Load and LoadFrom methods.

I'm unfortunately not a maintainer, was just asking a question while browsing the repo. @maxekman needs to have a look at this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants