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

Deduplicate CreationRequestReceived events #381

Merged
merged 2 commits into from
Apr 22, 2024

Conversation

sebastianburckhardt
Copy link
Member

Discovered more fallout from EH internal event duplication (as first observed in #379).

Specifically, it is possible for a creation request to be processed twice, thereby starting a duplicate work item, and this duplicate work item later creates corruption in the history (multiple execution started events) since both work items have the same execution id.

To fix this I stores the request id of the creation request that created the orchestration instance, and use that to filter duplicate deliveries of the same event.

src/DurableTask.Netherite/PartitionState/InstanceState.cs Outdated Show resolved Hide resolved
@@ -211,6 +222,8 @@ void DeleteState(EffectTracker effects)
effects.AddDeletion(TrackedObjectKey.History(this.InstanceId));

this.OrchestrationState = null;
this.OrchestrationStateSize = 0;
Copy link
Member

Choose a reason for hiding this comment

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

was not setting this to 0 this causing an issue before?

@@ -39,12 +42,19 @@ public override string ToString()

public override void Process(CreationRequestReceived creationRequestReceived, EffectTracker effects)
{
if (creationRequestReceived.EventIdString == this.CreationRequestEventId)
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if there's a way to deduplicate EH earlier in the pipeline, so that we don't have to sprinkle de-duplication logic in various places. Not sure if there's some kind of counter we can use to determine that we've processed all events up to some ID, or something like that. Hard to say, I could also see the possibility that we need to handle each case separately.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think there is anything we can do here, because we cannot keep track of the lifetime of clients.

@davidmrdavid
Copy link
Member

@sebastianburckhardt: I'd love to get this merged for the upcoming release, and I think we can mostly ignore my nitpicks. If this looks good to merge, please go ahead!

@sebastianburckhardt sebastianburckhardt merged commit 40a1f8c into main Apr 22, 2024
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.

2 participants