-
Notifications
You must be signed in to change notification settings - Fork 224
Align Java API with other languages #1560
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
base: master
Are you sure you want to change the base?
Conversation
f7ef1a9
to
592b883
Compare
@mcruzdev The patch coverage is failing. Looks like some more unit tests are needed. |
LGTM. Can be merged once the patch succeedes |
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.
@mcruzdev thanks a lot for looking into this.
I would like to request changes and suggest some improvements:
- There is another method in the DaprWorkflowClient which uses instance
purgeInstance
I think it should be renamed topurgeWorkflow
to stay consistent with other API changes. - We should add a new interface named
WorkflowState
that should be similar toWorkflowInstanceStatus
and we should ensure that instead ofgetInstanceId
we usegetWorkflowId
- Once you have the new interface you should adjust the implementation to ensure that the new methods use the new interface.
- You will also have to add an implementation similar to
DefaultWorkflowInstanceStatus
but forWorkflowState
something likeDefaultWorkflowState
. - One more thing I think we should adjust Java Docs to use
workflowId
instead ofinstanceId
so we have a more clean API description.
I know it is a little bit more work, but I think it is totally worth it if we want to align with other SDKs.
Thank you 🙇
Signed-off-by: Matheus Cruz <[email protected]>
7d40e76
to
96eb34e
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1560 +/- ##
============================================
+ Coverage 76.91% 78.41% +1.49%
- Complexity 1592 1928 +336
============================================
Files 145 216 +71
Lines 4843 5860 +1017
Branches 562 656 +94
============================================
+ Hits 3725 4595 +870
- Misses 821 927 +106
- Partials 297 338 +41 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Hi @artur-ciocanu, I applied all requested changes, let me know if we have something wrong :) |
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.
@mcruzdev love it ❤️ ! I have left a tiny comment related to docs. You should make sure that those are aligned with your changes.
Also we need to make sure the build is 🟢.
System.out.printf("Started new workflow instance with random ID: %s%n", instanceId); | ||
|
||
System.out.println(separatorStr); | ||
System.out.println("**GetInstanceMetadata:Running Workflow**"); |
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.
We should change the example to use WorkflowState
System.out.println(separatorStr); | ||
System.out.println("**WaitForInstanceCompletion**"); | ||
System.out.println("**waitForWorkflowCompletion**"); | ||
try { |
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.
We should change the example to use WorkflowState
System.out.printf("Started new workflow instance with random ID: %s%n", instanceId); | ||
|
||
System.out.println(separatorStr); | ||
System.out.println("**GetInstanceMetadata:Running Workflow**"); |
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.
We should change the example to use WorkflowState
System.out.println(separatorStr); | ||
System.out.println("**WaitForInstanceCompletion**"); | ||
System.out.println("**waitForWorkflowCompletion**"); | ||
try { |
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.
We should change the example to use WorkflowState
@mcruzdev could you please check the test you added. I see these details in the build logs:
And then we have a long list of these:
And eventually the task timeouts. |
Description
This pull request aims to align the Java SDK Workflow API woth other languages (Python, C#, etc.)
What this pull request does:
waitForInstanceStart
,getInstanceState
andwaitForInstanceCompletion
.waitForWorkflowStart
,getWorkflowState
andwaitForWorkflowCompletion
)Issue reference
Closes #1554
Please reference the issue this PR will close: #[issue number]
Checklist
Please make sure you've completed the relevant tasks for this PR, out of the following list: