[WIP] Crash resilience ProfOfConcept implementation on MS SQL #4
Conversation
Allow to run integration tests locally
|
The base idea: after successfull publishing write information about range of published events (min version, max version, aggregateId). This implementation is EventPersistance agnostic. This implementation is fully separate from existing code - so no breaking changes, etc. |
|
@leotsarev , could you review and discuss? |
Source/EventFlow.MsSql/ReliablePublish/MsSqlPublishVerificator.cs
Outdated
Show resolved
Hide resolved
MaxShoshin
force-pushed
the
crash-resilience-poc
branch
from
6b201d5 to
5b58b52
Compare
| using EventFlow.Aggregates; | ||
| using EventFlow.ReadStores; | ||
|
|
||
| namespace EventFlow.PublishRecovery |
There was a problem hiding this comment.
I think it could be WAY easier if we just can resolve IRecoveryHandler<TReadModel> from resolver
|
|
||
| await _msSqlConnection.ExecuteAsync( | ||
| Label.Named("publishlog-commit"), | ||
| CancellationToken.None, // Unable to Cancel |
There was a problem hiding this comment.
Why we could not cancel here? If some callers do not have cancellation token, they could pass None
There was a problem hiding this comment.
Why you should want to cancel writing info about successfully published to side effects?
There was a problem hiding this comment.
Same question goes for MarkVerifiedAsync (and basically every update), isn't it? Let's be consistent
I think "should I be allowed to cancel" is not for persistence to decide....
| { | ||
| public interface IReadModelRecoveryHandler | ||
| { | ||
| Task RecoverFromShutdownAsync( |
There was a problem hiding this comment.
Why we are inconsistent in signatures? I think it's ok to just have "if you throw, you failed to recover". I don't like API that's tell us "we could be fallible", because we want user to try to get infallible API ;-)
| IReadOnlyCollection<IDomainEvent> eventsForRecovery, | ||
| CancellationToken cancellationToken); | ||
|
|
||
| Task<bool> RecoverFromErrorAsync( |
There was a problem hiding this comment.
Also, I'm highly in doubt that we really need to distinct methods. May be parameter like "RecoveryReason" will fit better, cause usually people will like to implemement both methods they same.
|
|
||
| namespace EventFlow.PublishRecovery | ||
| { | ||
| public sealed class PublishVerificator : IPublishVerificator |
There was a problem hiding this comment.
I don't reviewed this file cause I hope to do it later in separate review
| { | ||
| public interface IReliableMarkProcessor | ||
| { | ||
| Task MarkEventsPublishedAsync(IReadOnlyCollection<IDomainEvent> domainEvents); |
There was a problem hiding this comment.
I don't think we need this abstraction actually now.
We now have only two reasons left for that:
- To inject a NOP implementation (it could be easily done by injecting NOP persistence)
- To group domain events by aggregate (see my persistence comment)
| _msSqlConnection = msSqlConnection; | ||
| } | ||
|
|
||
| public async Task MarkPublishedAsync(IIdentity aggregateIdentity, IReadOnlyCollection<IDomainEvent> domainEvents) |
There was a problem hiding this comment.
I think that by taking aggregateIdentity parameter you are leaking your implementation into interface.
This could be determined from domainEvents and some other implementations could be fine with marking events from different aggregateIdentity in one roundtrip.
There was a problem hiding this comment.
However, I don't think it will be easy to completely remove this from interface.
There was a problem hiding this comment.
It's some kind of 'guarantee' that all domain events should related with one aggregate...
There was a problem hiding this comment.
Why we need this kind of guarantee?
| var item = new PublishLogItem | ||
| { | ||
| AggregateId = aggregateIdentity.Value, | ||
| MinAggregateSequenceNumber = domainEvents.Min(x => x.AggregateSequenceNumber), |
There was a problem hiding this comment.
We got an assumption here that events are sequential. I thinks thats fine, but probably we should throw if assumption fails
| logItems); | ||
| } | ||
|
|
||
| public async Task MarkVerifiedAsync( |
There was a problem hiding this comment.
Same here — we are leaking implementation detail of having EventFlowPublishVerifyState into interface.
May be could just call MarkPublishedAsync from verificator and have some kind of Compact method which is implementation detail and could be MS SQL specific.
| GlobalPosition newVerifiedPosition, | ||
| CancellationToken cancellationToken) | ||
| { | ||
| await _msSqlConnection.ExecuteAsync( |
There was a problem hiding this comment.
I'm started to miss why no transaction here is okay. May be we need to
WHERE LastVerifiedPosition <@LastVerifiedPosition to be sure
|
|
||
| namespace EventFlow.PublishRecovery | ||
| { | ||
| public interface IReliablePublishPersistence |
There was a problem hiding this comment.
See comments in implementation about this interface.
| { | ||
| public interface IReliablePublishPersistence | ||
| { | ||
| Task MarkPublishedAsync(IIdentity aggregateIdentity, IReadOnlyCollection<IDomainEvent> domainEvents); |
There was a problem hiding this comment.
Also, we probably need to method to determine verification watermark for given aggregate for preventing publishing events out-of-order for aggregates. And yes, that's here where separate persistence for publish mark will add +1 read per command.
There was a problem hiding this comment.
I don't think we have to implement prevent events out of order right away (I'm rather not), but if we gonna publish separate PRs we need to include this in interface from first shot. It's thing for interfaces that you won't have 2nd chance to add another method.
No description provided.