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

Optionally disable sequence numbers using optimistic locking (etags) #362

Draft
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

mumby0168
Copy link
Collaborator

@mumby0168 mumby0168 commented Nov 2, 2023

  • 1. Introduces a new way to sequence events without optimistic locking
  • 2. Adds the ability to use in memory testing for events
  • 3. Adds the ability to test change feed projections using an in-memory implementation
  • 4. Shows how the in-memory library can be used with functional tests via a sample
  • document non-sequenced option along with the aggregate root settings with the config option
  • document how the in-memory testing works


namespace Microsoft.Azure.CosmosEventSourcing.Options;

public class CosmosEventSourcingOptions
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll need triple slash for these public APIs.

IEventStore<TEventItem> where TEventItem : EventItem
{
private readonly IOptionsMonitor<CosmosEventSourcingOptions> _optionsMonitor = optionsMonitor;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This field isn't needed, you can access the optionsMonitor anywhere within the partial class. These are primary constructors.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ahhh that makes sense now, I'll update this cheers!

@@ -22,9 +22,12 @@ internal partial class DefaultEventStore<TEventItem>
return;
}

if (eventItems.Count(x => x.EventName is nameof(AtomicEvent)) is not 1)
if(_optionsMonitor.CurrentValue.IsSequenceNumberingDisabled is false)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if(_optionsMonitor.CurrentValue.IsSequenceNumberingDisabled is false)
if (optionsMonitor.CurrentValue.IsSequenceNumberingDisabled is false)

typeof(TEventItem),
aggregateRoot.AtomicEvent,
partitionKey) as TEventItem);
if(_optionsMonitor.CurrentValue.IsSequenceNumberingDisabled is false)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if(_optionsMonitor.CurrentValue.IsSequenceNumberingDisabled is false)
if (optionsMonitor.CurrentValue.IsSequenceNumberingDisabled is false)

switch (domainEvent)
{
case ReplayableEvent:
ReplayedEvents++;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any race condition concerns with multiple threads calling apply at the same time, and reads of the ReplayedEvents?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes there will be, kind of a long story to this change, I plan to update the docs alongside this as well. But in short, the sequence number and the strong consistency provided by the atomic event is great in a single region scenario.

However, when we have been assessing the ability to enable multi region writes where traffic would be load balanced across two regions both writing to there local cosmos db data store, we could not guarantee that this sequence number remains in sync and in some use cases for event sourcing it doesn't really matter. The plan here is to handle simultaneous writes, as long as that event is valid at the point in time we are going to let it go ahead.

This diagram is covering our use case, but I am still fleshing out all the details of whether this is 100% suitable via some POCs.

Screenshot 2023-11-02 at 18 05 29

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm good with it if you are, let's :shipit:

y is EventItemPartitionKeyAttribute))
.ToList();

switch (partitionKeyProperties.Count)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prefer switch expressions.

@@ -36,4 +39,41 @@ eventItem.DomainEvent switch
null => null,
_ => throw new ArgumentOutOfRangeException()
};

public static TAggregateRoot Replay<TAggregateRoot, TEventItem>(this IEnumerable<TEventItem> events)
where TEventItem : EventItem
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
where TEventItem : EventItem
where TEventItem : EventItem

public static IEnumerable<TEventItem> SetCorrelationId<TEventItem>(
this IEnumerable<TEventItem> eventItems,
IContextService contextService)
where TEventItem : EventItem
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
where TEventItem : EventItem
where TEventItem : EventItem

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

Successfully merging this pull request may close these issues.

None yet

2 participants