-
Notifications
You must be signed in to change notification settings - Fork 221
Remove try with resource for Worfklow Worker Examples and use latest changes from durabletask-java #1337
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
Conversation
Signed-off-by: Siri Varma Vegiraju <[email protected]>
69817e0
to
1ae4fcb
Compare
@cicoyle @salaboy @artur-ciocanu Can you folks please review this PR ? this is to incorporate the durabletask-java changes. Build will pass once we incorporate the new release. |
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.
thanks for adding the ternary and the followup here 👍🏻 lgtm. just need to wait for the dapr/durabletask-java release to show up in maven central and let CI run green then we can merge
@siri-varma and @cicoyle I really the like the idea of being able to provide an @siri-varma could you please provide a little bit more details. Thank you. |
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.
@siri-varma LGTM, I get it why we need ExecutorService
changes, but I am not following why we need to remove AutoClosable
this.executorService.shutdownNow(); | ||
} | ||
} catch (InterruptedException ex) { | ||
Thread.currentThread().interrupt(); | ||
} | ||
} |
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.
Please add a new line
@artur-ciocanu The durabletask-java SDK does not manage the gRPC channel when one is externally supplied. In our case, since the java-sdk provides the channel, we are responsible for managing its lifecycle. So the try with resource was a no op today. To fix that, I’ve added close() and shutdown() methods in WorkflowRuntime to ensure proper cleanup. As a result, in case of workers, since the gRPC server needs to run continuously, we cannot use try-with-resources — doing so would automatically invoke close(), which would prematurely shut down the server. Also, the AutoCloseable is still used in the WorkflowRuntime. I just removed the try with resource from the Worker examples because of the above reason. |
@cicoyle I will pull in your changes once they are merged. |
Signed-off-by: siri-varma <[email protected]>
Signed-off-by: siri-varma <[email protected]>
05214c3
to
eda5253
Compare
Closing this. All the commits wen in this PR #1336 |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1337 +/- ##
============================================
+ Coverage 76.91% 77.28% +0.36%
- Complexity 1592 1765 +173
============================================
Files 145 204 +59
Lines 4843 5388 +545
Branches 562 590 +28
============================================
+ Hits 3725 4164 +439
- Misses 821 908 +87
- Partials 297 316 +19 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Description
Please explain the changes you've made
Issue reference
We strive to have all PR being opened based on an issue, where the problem or feature have been discussed prior to implementation.
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: