awesomeness icon indicating copy to clipboard operation
awesomeness copied to clipboard

[RFC] remove $identityMap from AggregateRepository

Open sandrokeil opened this issue 8 years ago • 6 comments

I would like to discuss to remove the caching of aggregates via identityMap. The main problem with this are long running processes like projections. If a projection have to load the aggregate, the wrong state can be loaded. The user has also to ensure not to run in memory exhausting. This is error prone too.

Assume that PHP process PhpA is a long running projection which have to load the aggregate because it replaces the whole read model entry for this aggregate and not only the data from the event. The PHP process PhpB is our write process.

PhpB -> event1
PhpA -> event1
PhpB -> event2
PhpA -> event2 (but it gets the state of event1 via repository->get() because of identity map)

sandrokeil avatar Apr 23 '18 16:04 sandrokeil

I'm okay with removing, what do you think @codeliner ?

prolic avatar Apr 23 '18 16:04 prolic

Can second removal. With command handlers in background jobs an AR effectively becomes bound to a specific process (unless the map is cleared).

  1. Process A loads AR, pushes new version 2
  2. Process B loads AR, pushes new version 3
  3. Process A has AR in cache, tries to push new version, ConcurrencyException but old version is kept in map
  4. Command is retried, fails until it reaches process B

We "solved" this by wrapping an repositories save function to just catch concurrency exceptions, clear the cache, and rethrow the exception for all our repos. If caching is required this should be delegated to a strategy or facade imho.

fritz-gerneth avatar Apr 23 '18 21:04 fritz-gerneth

Maybe we should do this in current version too, if accepted.

sandrokeil avatar Apr 24 '18 07:04 sandrokeil

At least some flag / option on the repo to disable it would be helpful indeed :) Leave it on by default for BC.

fritz-gerneth avatar Apr 24 '18 07:04 fritz-gerneth

@sandrokeil @fritz-gerneth

At least some flag / option on the repo to disable it would be helpful indeed :) Leave it on by default for BC.

This can be done immediately, as there is no BC, just submit a PR incl. tests and I will merge and release this change.

prolic avatar Apr 24 '18 08:04 prolic

yeah, we can remove identity map with next major release. In earlier versions we used it to keep the illusion that an aggregate repository can be an in-memory collection (no $repo->save($ar) call needed), but current repository interface has a save method and a unit of work is not required/no longer used.

Additional flag to deactivate identity map in current version sounds good, too.

codeliner avatar Apr 27 '18 20:04 codeliner