search

gautema / CQRSlite

A lightweight framework to help creating CQRS and Eventsourcing applications in C#
Other
1.1k stars 266 forks source link

CacheRepository Getting is not really caching? #75

Closed p10tyr closed 6 years ago

p10tyr commented 6 years ago

I have been modifying the framework to meet some of our requirement, specifically we are not interested in having the Version (Sequence Number) stored on the aggregate as a Version. We using EventStore and that handles that for us.

So I have moved all your tests over to my project and had a problem with one test.

When_saving_two_aggregates_in_parallel.Should_distibute_events_correct

This uses the CacheRepository as in your source and the result coming out for me was 5140 instead of 100. Now I knew exactly that it was re applying the events, on top of an Aggregates correct state.. but was not sure where any why.

Looking at this code CacheRepository.Task<T> Get<T>(Guid aggregateId, CancellationToken cancellationToken = default(CancellationToken))

To me it seems like the caching feature is redundant.

To be clear, your events are stored in a dictionary where mine are coming from EventStore - I want to reduce these calls as to EventStore as they are expensive. But this is not the problem.

The problem is that it gets the Aggregate from MemeryCache, Get the events, and then 99% of the time discards it, then goes and gets everything again.

The test in your case is valid, it is always 100 - But you are not testing to see if its using the cached version or did it go and get everything again. I think if you were to adjust it to check that you come to find that your cache is always discarding the Aggregate.

In my case, the Aggregate is cached and since I removed the Version check, it just replayed the events again. So if the state was 10, it replayed 10 events again, making it 20, instead of 10. So this is how I found the issue in your side. I will do some other things to make sure the Aggregate is in the correct state. But now I do not replay the events and cache works, and the results are 100 as expected.

gautema commented 6 years ago

Hi. Sorry for the late reply, but I've been on summer holiday.

You are right that it will most likely not be any new events. But if the aggregate is cached on several servers, it might be. If you don't want this behaviour, don't use the CacheRepository. It seems to me that you have implemented the IEventStore.Get-method wrong. It should return all events after the fromVersion. So the eventstore should return either aggregate.Version + 1 or an empty list. I have tried to change the test as you proposed, but I still get the response I expect. If you still think there is a bug, please attach some code showing the issue as I can't reproduce it.

p10tyr commented 6 years ago

Hi - I have realised I made a mistake with the Version part and made it as you have it.

I understand your test will still result in success, but are you checking if it returns the cached version, when you expect it - And a new version when you expect it.

All I realised was that when I expected it to be cached, it was actually going to Evenstore even though nothing changed.