-
Notifications
You must be signed in to change notification settings - Fork 21
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
Conversation
|
There was a problem hiding this 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.
161cae1
to
b0e953c
Compare
There was a problem hiding this 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.
b0e953c
to
ca3359f
Compare
@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 |
There was a problem hiding this 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!
StartSignal = RequestLockSignalName, | ||
StartSignalArgs = new object[] { new LockRequest(activityInfo.WorkflowId, input.AcquireLockSignalName, input.LockTimeout), }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I opened temporalio/sdk-dotnet#134
ca3359f
to
3a1babb
Compare
3a1babb
to
e9e1096
Compare
Thanks for the update! Gonna merge #39 first if that's ok (unsure if it'll merge cleanly). |
Resolved conflict (using GH's built-in conflict editor for the first time, was interesting). Merging once CI passes. Thanks again! |
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