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

Workflow Mutex Sample #32

Merged
merged 2 commits into from
Nov 1, 2023
Merged

Workflow Mutex Sample #32

merged 2 commits into from
Nov 1, 2023

Conversation

devbased
Copy link
Contributor

@devbased devbased commented Sep 2, 2023

What was changed

Added a new sample showcasing how to implement a Mutex Workflow for managing concurrent access to a shared resource.

NOTE: Since the required SDK version (the one with workflow history info) hasn't been published yet, I had to add links to the SDK in TemporalioSamples.sln and Directory.build.props

@CLAassistant
Copy link

CLAassistant commented Sep 2, 2023

CLA assistant check
All committers have signed the CLA.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Copy link
Member

@cretz cretz left a comment

Choose a reason for hiding this comment

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

Added some comments, but overall this is the same as I was thinking. I like the idea of wrapping as a utility.

Directory.Build.props Outdated Show resolved Hide resolved
src/Mutex/Impl/ILockHandle.cs Show resolved Hide resolved
src/Mutex/Impl/Internal/MutexWorkflow.workflow.cs Outdated Show resolved Hide resolved
src/Mutex/Activities.cs Show resolved Hide resolved
src/Mutex/Impl/Internal/LockRequest.cs Outdated Show resolved Hide resolved
src/Mutex/Impl/Internal/MutexWorkflowInput.cs Outdated Show resolved Hide resolved
src/Mutex/Impl/Internal/MutexWorkflow.workflow.cs Outdated Show resolved Hide resolved
src/Mutex/README.md Outdated Show resolved Hide resolved
src/Mutex/WorkflowWithMutex.workflow.cs Outdated Show resolved Hide resolved
src/Mutex/WorkflowWithMutexInput.cs Outdated Show resolved Hide resolved
src/Mutex/Program.cs Outdated Show resolved Hide resolved
@devbased devbased force-pushed the mutex-workflow branch 3 times, most recently from 161cae1 to b0e953c Compare September 5, 2023 16:52
Copy link
Member

@cretz cretz left a comment

Choose a reason for hiding this comment

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

Looks good. Marking approved. I think it can be simplified bit, though I know that file and package sprawl is kinda the .NET way. Approving for CI run, but of course it will fail due to the relative-path dep, no worry there, we can leave this PR open for a bit.

@devbased
Copy link
Contributor Author

devbased commented Sep 5, 2023

@cretz I reduced the number of files, but I didn't refactor the inputs into nested classes; instead, I placed them outside in the same file where they are being used. Additionally, I provided a bit more detailed description of the project's structure in the README.md

Copy link
Member

@cretz cretz left a comment

Choose a reason for hiding this comment

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

Looks good!

I didn't refactor the inputs into nested classes; instead, I placed them outside in the same file where they are being used.

I think it's clearer as nested classes (I like the type-per-file rule), but not a big deal I suppose.

Will merge when SDK is updated. Thanks!

src/Mutex/Activities.cs Show resolved Hide resolved
Comment on lines 31 to 32
StartSignal = RequestLockSignalName,
StartSignalArgs = new object[] { new LockRequest(activityInfo.WorkflowId, input.AcquireLockSignalName, input.LockTimeout), },
Copy link
Member

Choose a reason for hiding this comment

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

src/Mutex/Impl/LockRequest.cs Outdated Show resolved Hide resolved
@devbased devbased changed the title [DRAFT] Workflow Mutex Sample Workflow Mutex Sample Nov 1, 2023
@devbased devbased marked this pull request as ready for review November 1, 2023 04:15
@cretz
Copy link
Member

cretz commented Nov 1, 2023

Thanks for the update! Gonna merge #39 first if that's ok (unsure if it'll merge cleanly).

@cretz
Copy link
Member

cretz commented Nov 1, 2023

Resolved conflict (using GH's built-in conflict editor for the first time, was interesting). Merging once CI passes. Thanks again!

@cretz cretz merged commit d3fdf7c into temporalio:main Nov 1, 2023
5 checks passed
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.

3 participants