-
Notifications
You must be signed in to change notification settings - Fork 207
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
Initial Workflow SDK #857
Initial Workflow SDK #857
Conversation
Putting this on our radars @cgillum , @DeepanshuA , @RyanLettieri |
@cgillum , @DeepanshuA , @RyanLettieri, @nyemade-uversky This PR is almost done in terms of foundations. Would love a quick review before we add a bunch more methods if you have time this week. |
Signed-off-by: Bill DeRusha <billderusha@microsoft.com>
Signed-off-by: Hannah Kennedy <hakenned@microsoft.com>
Signed-off-by: Hannah Kennedy <hakenned@microsoft.com>
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.
This is a great start! Some comments below.
examples/src/main/java/io/dapr/examples/workflows/DemoWorkflow.java
Outdated
Show resolved
Hide resolved
sdk-workflows/src/main/java/io/dapr/workflows/runtime/WorkflowRuntime.java
Outdated
Show resolved
Hide resolved
sdk-workflows/src/main/java/io/dapr/workflows/runtime/WorkflowRuntime.java
Show resolved
Hide resolved
examples/src/main/java/io/dapr/examples/workflows/DemoWorkflowService.java
Outdated
Show resolved
Hide resolved
sdk-workflows/src/main/java/io/dapr/workflows/client/DaprWorkflowClient.java
Outdated
Show resolved
Hide resolved
sdk-workflows/src/main/java/io/dapr/workflows/runtime/WorkflowContext.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Bill DeRusha <444835+bderusha@users.noreply.github.com>
Signed-off-by: Bill DeRusha <444835+bderusha@users.noreply.github.com>
@cgillum Thanks for the feedback! I pushed up a new round of changes to address them |
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.
A few additional comments/questions.
sdk-workflows/src/main/java/io/dapr/workflows/runtime/DaprWorkflowContextImpl.java
Outdated
Show resolved
Hide resolved
sdk-workflows/src/main/java/io/dapr/workflows/runtime/WorkflowRuntime.java
Outdated
Show resolved
Hide resolved
examples/src/main/java/io/dapr/examples/workflows/DemoWorkflowClient.java
Outdated
Show resolved
Hide resolved
sdk-workflows/src/test/java/io/dapr/workflows/runtime/DaprWorkflowContextImplTest.java
Outdated
Show resolved
Hide resolved
sdk-workflows/src/test/java/io/dapr/workflows/runtime/DaprWorkflowContextImplTest.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Bill DeRusha <444835+bderusha@users.noreply.github.com>
Thanks for the quick turnaround, @cgillum. I believe I have addressed all active comments. Let me know if you have any more suggestions. |
Signed-off-by: Bill DeRusha <444835+bderusha@users.noreply.github.com>
Signed-off-by: Bill DeRusha <444835+bderusha@users.noreply.github.com>
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.
Just a few small things/questions.
sdk-workflows/src/main/java/io/dapr/workflows/runtime/DaprWorkflowContextImpl.java
Outdated
Show resolved
Hide resolved
sdk-workflows/src/main/java/io/dapr/workflows/runtime/OrchestratorWrapper.java
Outdated
Show resolved
Hide resolved
sdk-workflows/src/main/java/io/dapr/workflows/runtime/WorkflowRuntime.java
Outdated
Show resolved
Hide resolved
sdk-workflows/src/main/java/io/dapr/workflows/runtime/WorkflowRuntime.java
Outdated
Show resolved
Hide resolved
Thanks @cgillum Pushed up a quick set of changes based on your feedback. Is there anyone else we need to loop into this PR for it to land? It feels like we're getting close here |
@bderusha this PR looks good to me. However, I don't have permissions to merge it as I'm not a maintainer for the Dapr Java SDK (my signoffs don't count either). We'll need @dapr/approvers-java-sdk and @dapr/maintainers-java-sdk to take a look. FYI @nyemade-uversky in case we need to ping relevant folks directly. |
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 like a good start! I just had a few comments about dependencies that were added to POMs (looks like perhaps dependencies got added in places they don't need to be).
Signed-off-by: Bill DeRusha <444835+bderusha@users.noreply.github.com>
Thanks @johnewart. Good catch on those dependencies. Should be all set now. |
@dapr/maintainers-java-sdk would you be able to take a look at this PR? |
@cgillum I don't know if this is the time or the place, but can I ask that code examples are included that demonstrate how to unit test a Workflow from an end users perspective i.e how to mock Activity calls, timers, continueAsNew etc etc |
@artursouza @mukundansundar Could you help trigger build again, please? I think I have resolved all the build issues now |
@artursouza, @mukundansundar and @bderusha would it be possible to get this PR merged this week? Looks like we're waiting on the build to pass, is that the only ting blocking this pr rn? |
@nyemade-uversky There is a single failing test for |
Thank you for rerunning the tests @mukundansundar Looks like that failure was intermittent. All tests are passing now. FYI @nyemade-uversky |
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.
This is great. I have provided some feedback.
Overall, I think it should be built using more of the main SDK connectivity - otherwise it will require duplication of code to handle connection to sidecar.
Last but not least, please, consider making this a separate project under dapr-sandbox org - instructions at https://github.com/dapr-sandbox/
I do think I should continue helping in the review to keep consistent user experience for the Java SDKs since not all decisions are documented.
sdk-workflows/src/main/java/io/dapr/workflows/client/DaprWorkflowClient.java
Outdated
Show resolved
Hide resolved
sdk-workflows/src/main/java/io/dapr/workflows/runtime/DaprWorkflowContextImpl.java
Outdated
Show resolved
Hide resolved
sdk-workflows/src/main/java/io/dapr/workflows/runtime/Workflow.java
Outdated
Show resolved
Hide resolved
sdk-workflows/src/main/java/io/dapr/workflows/runtime/WorkflowContext.java
Outdated
Show resolved
Hide resolved
sdk-workflows/src/main/java/io/dapr/workflows/runtime/WorkflowContext.java
Outdated
Show resolved
Hide resolved
sdk-workflows/src/main/java/io/dapr/workflows/runtime/WorkflowRuntime.java
Outdated
Show resolved
Hide resolved
sdk-workflows/src/main/java/io/dapr/workflows/runtime/WorkflowRuntime.java
Outdated
Show resolved
Hide resolved
sdk-workflows/src/main/java/io/dapr/workflows/runtime/WorkflowRuntime.java
Outdated
Show resolved
Hide resolved
@artursouza Thank you for the feedback. The code comments all make sense. I am curious to learn more about The context I am thinking about here is that other SDKs have early workflow support in their main repos (python-sdk, dotnet-sdk) and of course the workflow engine itself is in the mainline dapr/dapr. At first blush it feels like this feature is no different from other alpha APIs introduced to the dapr runtime and then supported by the SDKs. But I am admittedly just learning about the FYI I will be on vacation most of July and slow to respond, so I am tagging others who might be interested in this while I'm out. |
Making this a dapr-sandbox repo would need to be consistent with other SDKs, so all of them would move the WF part to it. In this case, there is a different since it is for composing workflow and not simply an Alpha API. The wf invocation is a simple Alpha API and it should still be here. The main driving factor for this is that this is at a different maturity level and a new artifact. The fact that it is versioned differently from the other artifacts of the SDK says it all. That is why I am leaning torwards this being a separate SDK. For this to be merged in this repo, we would also need to update all the release scripts to handle specific versioning for this artifact. See https://github.com/dapr/java-sdk/wiki/Release-Process |
/cc @brunoborges do you mind taking a look at this PR too? It is from Msft. |
@artursouza - @nyemade-uversky shared with us re: letting this PR merge into the Dapr repo. We have Bill out of office for the month of July. Considering the feedback suggested will involved some deep changes, we (Bill and I) agreed that it was best for Bill to pick this back up in August. I hope that works for the team. |
Hey Artur, sorry but I'm afraid I don't have the necessary knowledge of the Dapr SDK, nor the time, to provide meaningful feedback here. |
That works for me. Yes, the PR can be added here. |
@NarmathaBala @bderusha |
@mukundansundar I just got back from some time off and am planning to make progress on the PR this week. |
Signed-off-by: Bill DeRusha <444835+bderusha@users.noreply.github.com>
@mukundansundar @artursouza I believe I have addressed all outstanding comments. Please take a look when you get a chance. |
Signed-off-by: Bill DeRusha <444835+bderusha@users.noreply.github.com>
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.
it is a gread PR!
I put some very detail comments about the code implementation.
Let's make it perfect.
sdk-workflows/src/main/java/io/dapr/workflows/client/DaprWorkflowClient.java
Outdated
Show resolved
Hide resolved
sdk-workflows/src/test/java/io/dapr/workflows/client/DaprWorkflowClientTest.java
Show resolved
Hide resolved
sdk-workflows/src/main/java/io/dapr/workflows/runtime/OrchestratorWrapper.java
Outdated
Show resolved
Hide resolved
sdk-workflows/src/main/java/io/dapr/workflows/runtime/OrchestratorWrapper.java
Outdated
Show resolved
Hide resolved
sdk-workflows/src/main/java/io/dapr/workflows/runtime/OrchestratorWrapper.java
Show resolved
Hide resolved
sdk-workflows/src/main/java/io/dapr/workflows/runtime/Workflow.java
Outdated
Show resolved
Hide resolved
sdk-workflows/src/main/java/io/dapr/workflows/runtime/WorkflowRuntime.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Bill DeRusha <444835+bderusha@users.noreply.github.com>
examples/src/main/java/io/dapr/examples/workflows/DemoWorkflow.java
Outdated
Show resolved
Hide resolved
sdk-workflows/src/main/java/io/dapr/workflows/DaprWorkflowContextImpl.java
Outdated
Show resolved
Hide resolved
sdk-workflows/src/main/java/io/dapr/workflows/DaprWorkflowContextImpl.java
Outdated
Show resolved
Hide resolved
sdk-workflows/src/main/java/io/dapr/workflows/WorkflowContext.java
Outdated
Show resolved
Hide resolved
sdk-workflows/src/main/java/io/dapr/workflows/client/DaprWorkflowClient.java
Outdated
Show resolved
Hide resolved
} | ||
|
||
@Override | ||
public TaskOrchestration create() { |
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.
If all the API is blocking for DurableTask, then ProjectReactor does not add any value and makes things more complicated by giving the illusion of async model. Does DurableTask have an async API or is it all blocking?
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.
@artursouza DurableTask does not expose any async methods in the client or runtime. The only async API is for some of the Orchestration and Activity Context classes which return custom Task classes which wrap java.util.concurrent.CompletableFuture
objects.
I am new to ProjectReactor, so I don't have a good sense for effort VS value of trying to convert those context methods to mono. What are your thoughts?
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.
DurableTask does not expose any async methods in the client or runtime.
This is so bad! So since DurableTask is blocking, then we use reactor in java sdk won't bring any value but make it complex.
It seems that we need to give up Mono and use normal blocking call.
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 agree. We should keep it blocking until the DurableTask SDK addresses this: microsoft/durabletask-java#160
sdk-workflows/src/main/java/io/dapr/workflows/runtime/WorkflowRuntime.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Bill DeRusha <444835+bderusha@users.noreply.github.com>
Signed-off-by: Bill DeRusha <444835+bderusha@users.noreply.github.com>
Signed-off-by: Bill DeRusha <444835+bderusha@users.noreply.github.com>
Codecov Report
@@ Coverage Diff @@
## master #857 +/- ##
============================================
+ Coverage 77.31% 77.49% +0.18%
- Complexity 1268 1304 +36
============================================
Files 117 123 +6
Lines 3927 4013 +86
Branches 468 472 +4
============================================
+ Hits 3036 3110 +74
- Misses 652 662 +10
- Partials 239 241 +2
|
Description
This PR covers just enough of the authoring and management APIs to allow for anyone to be able to add the rest of them without requiring much/any coordination around structure.
Design decisions of note:
sdk-workflows
is implemented as a separate optional package (like actors)WorkflowRuntime.getInstance().registerWorkflow(...)
Issue reference
#839
Checklist
Please make sure you've completed the relevant tasks for this PR, out of the following list: