-
Notifications
You must be signed in to change notification settings - Fork 141
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
Add shutdown API to JDS along with task handler #1376
base: main
Are you sure you want to change the base?
Add shutdown API to JDS along with task handler #1376
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1376 +/- ##
==========================================
- Coverage 19.09% 18.81% -0.28%
==========================================
Files 166 166
Lines 11062 11235 +173
==========================================
+ Hits 2112 2114 +2
- Misses 8950 9121 +171
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
My gut feeling is that bfa7b81 is somehow over-enginnering or is a result of some code debt. I would prefer for us to move all code to tokio first and then start handling async tasks forced shutdown. If this is a blocker for any role for "shutdown" function, I would wait with that until then.
We might endup eventually with the same design, but we would be more confident with it if we had all networking setup is handled first.
Yes, I agree this adds a significant amount of code and feels abit of overengineering, particularly for creating a construct to manage the lifecycle of child tasks. However, during the roles refactor, we should aim to design a mechanism that simplifies task lifecycle management as much as possible.That said, it’s critical to merge this now because, without it, shutting down the |
Dunno really. I understand the urgency but in the same time we lived with this for a bit now. Lets see what others think. |
Partially Closes: #1320
This PR introduces a shutdown API, similar to the implementation in JDC and TProxy. Additionally, to facilitate a graceful shutdown, we have added a task handler that manages the lifecycle of all tasks spawned within the
start
method.