-
Notifications
You must be signed in to change notification settings - Fork 549
refactor job hosting logic #5101
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
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.
Pull Request Overview
This PR refactors the job hosting logic to support dynamic allocation of threads/tasks. The primary goal is to enable adjusting the number of tasks without restarting the job hosting service while preserving all existing functionality.
Key changes:
- Replaces static thread allocation with dynamic task management using a concurrent dictionary to track running jobs targets
- Introduces real-time task scaling based on configurable targets per queue type
- Adds comprehensive logging and monitoring improvements for better observability
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
File | Description |
---|---|
JobHosting.cs | Complete refactor of job execution logic from static thread pool to dynamic task management with configurable targets |
ImportTests.cs | Adds E2E test to verify job dequeue behavior and polling frequency under the new dynamic system |
test/Microsoft.Health.Fhir.Shared.Tests.E2E/Rest/Import/ImportTests.cs
Outdated
Show resolved
Hide resolved
@brendankowitz Thanks for asking for AI review. I found 2 wrong AI comments. Hope they fix that. |
Changes look good, however one item we should consider is how the new behavior will shutdown the workers and restart them as needed vs. starting them and polling at startup. While possibly minimal, GC and task creation overhead could increase especially under high load with high number of workers. |
This is a valid concern for very high frequences, thousands of operations per second. I think it is far from what we deal here (single digit operations per second). I also performed full scale testing on $import. No noticeable increase in app resource utilization. |
Refactoring to support dynamic allocation of threads/tasks in job hosting. After this change number of tasks can be adjusted without restarting job hosting. All other current functionality is preserved.