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

Fix missing dedupe states for gRPC client server impl #2489

Merged
merged 2 commits into from
Jun 13, 2023

Conversation

jviau
Copy link
Contributor

@jviau jviau commented Jun 7, 2023

Fixes missing dedupe states in the gRPC server client when scheduling a new orchestration via IOrchestrationServiceClient. Without the dedupe states, we would effectively restart already running orchestrations when it was scheduled again.

Issue describing the changes in this PR

resolves #2466

Pull request checklist

  • My changes do not require documentation changes
    • Otherwise: Documentation PR is ready to merge and referenced in pending_docs.md
  • My changes should not be added to the release notes for the next release
    • Otherwise: I've added my notes to release_notes.md -- TODO
  • My changes do not need to be backported to a previous version
    • Otherwise: Backport tracked by issue/PR #issue_or_pr
  • I have added all required tests (Unit tests, E2E tests)
  • My changes do not require any extra work to be leveraged by OutOfProc SDKs
    • Otherwise: That work is being tracked here: #issue_or_pr_in_each_sdk
  • My changes do not change the version of the WebJobs.Extensions.DurableTask package
    • Otherwise: major or minor version updates are reflected in /src/Worker.Extensions.DurableTask/AssemblyInfo.cs
  • My changes do not add EventIds to our EventSource logs
    • Otherwise: Ensure the EventIds are within the supported range in our existing Windows infrastructure. You may validate this with a deployed app's telemetry. You may also extend the range by completing a PR such as this one.


return new OrchestrationStatus[0];
OverridableStates overridableStates = this.durableTaskOptions.OverridableExistingInstanceStates;
return overridableStates.ToDedupeStatuses();
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm confused. The method name is GetStatusesNotToOverride, but the value we're returning seems to be the opposite of this: overridableStates. Why is this logic correct? Assuming it is correct, can we rename something to make it obviously correct?

Copy link
Contributor Author

@jviau jviau Jun 12, 2023

Choose a reason for hiding this comment

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

It is a bit confusing, but it is public API so no changing it unfortunately. I think it becomes a bit clearer when you read the values of OverridableStates. AnyStates and NonRunningStates. So, it is kind of a double negative. Don't think I can change it in this PR.

    /// <summary>
    /// Represents options for different states that an existing orchestrator can be in to be able to be overwritten by
    /// an attempt to start a new instance with the same instance Id.
    /// </summary>
    public enum OverridableStates
    {
        /// <summary>
        /// Option to start a new orchestrator instance with an existing instnace Id when the existing
        /// instance is in any state.
        /// </summary>
        AnyState,

        /// <summary>
        /// Option to only start a new orchestrator instance with an existing instance Id when the existing
        /// instance is in a terminated, failed, or completed state.
        /// </summary>
        NonRunningStates,
    }

@jviau jviau requested a review from cgillum June 12, 2023 17:24
@cgillum
Copy link
Collaborator

cgillum commented Jun 12, 2023

I think this Java issue will get resolved by this PR as well: microsoft/durabletask-java#135

@jviau jviau merged commit 09a7c6f into Azure:dev Jun 13, 2023
18 checks passed
@jviau jviau deleted the fix-dedupe branch June 13, 2023 20:34
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.

Singleton out of process orchestrator two running at the same time
2 participants