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

Added Options to Configure Behavior When Starting a New Orchestrator With an Existing Instance Id #325

Closed
wants to merge 3 commits into from

Conversation

amdeel
Copy link
Contributor

@amdeel amdeel commented Oct 16, 2019

Added options to configure which states you want to override an existing orchestrator instance id when attempting to start a new instance.

Fixes this issue in the azure-functions-durable-extension repository
Azure/azure-functions-durable-extension#367

Copy link
Member

@cgillum cgillum left a comment

Choose a reason for hiding this comment

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

This is a good start, but I don't think it's sufficient to fix the problem. If multiple threads call CreateTaskOrchestrationAsync at the same time, they will all see that existingInstance is null and all threads will try to create new instances. Locks won't help because it's also possible that the function app is scaled out and these calls are coming from two different nodes. To make this effective in all cases, we also need to update the code that reads these messages.

I think the two places which would need to check for these "ExecutionStarted" messages are here and here.

We should also add one or more tests to validate this new behavior.

@@ -1244,6 +1244,12 @@ public async Task CreateTaskOrchestrationAsync(TaskMessage creationMessage, Orch
return;
}

if (existingInstance != null && !OverrideConditionsMet(existingInstance.OrchestrationStatus))
Copy link
Member

Choose a reason for hiding this comment

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

Could we achieve the same outcome if we simply depended on the existing dedupeStatuses parameter? I don't have an good answer for this that I can think of right now, but something to consider.

if (existingInstance != null && !OverrideConditionsMet(existingInstance.OrchestrationStatus))
{
// An instance in a state that is configured to not be overridable already exists
return;
Copy link
Member

Choose a reason for hiding this comment

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

I think we should throw an InvalidOperationException in this case. Otherwise customers might not realize that their attempt to start an orchestration was ignored.

@amdeel
Copy link
Contributor Author

amdeel commented Oct 25, 2019

Closing this PR. Decided to use dedupe status instead and modify code in azure-functions-durable-extension. Azure/azure-functions-durable-extension#987

@amdeel amdeel closed this Oct 25, 2019
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