Refactor _executable_task_instances_to_queued to make logic more readable#66878
Open
ashb wants to merge 1 commit into
Open
Refactor _executable_task_instances_to_queued to make logic more readable#66878ashb wants to merge 1 commit into
ashb wants to merge 1 commit into
Conversation
boring-cyborg
Bot
added
the
area:Scheduler
ashb
changed the title
ashb
added
the
changelog:skip
jedcunningham
approved these changes
May 13, 2026
|
|
||
| def _executable_task_instances_to_queued(self, max_tis: int, session: Session) -> list[TI]: | ||
| def _acquire_pool_capacity( | ||
| self, max_tis: int, session: Session |
Member
There was a problem hiding this comment.
Suggested change
| self, max_tis: int, session: Session | |
| self, max_tis: int, *, session: Session |
Or, convert it all to be kwarg only. Same with the others.
Contributor
There was a problem hiding this comment.
I think we had the rule with session as kwarg mainly for the @provide_session decorator which is not used here. So a session needs to be provided by caller anyway. So it is rather a nit.
This comment was marked as resolved.
This comment was marked as resolved.
…stances Split the monolithic `_executable_task_instances_to_queued` into two focused methods: - `_acquire_pool_capacity`: takes the advisory lock and reads pool utilisation via SELECT FOR UPDATE. Returns (pools, max_tis, starved_pools) so callers can short-circuit when all pools are full before doing any TI selection work. - `_select_task_instances_to_queue`: given pre-computed pool capacity, selects eligible SCHEDULED TIs and moves them to QUEUED. Accepts the pools dict and starved_pools set as parameters, making it directly testable without needing a real lock or DB pool read. `_critical_section_enqueue_task_instances` now calls these two methods in sequence, making the two-phase structure (acquire capacity, then select and queue) visible at the orchestration level. All test call sites updated to call `_select_task_instances_to_queue` directly with a `make_pool_stats()` helper, removing the dependency on pool row locking in unit tests.
ashb
force-pushed
the
scheduler-enqueue-readability
branch
from
007fd8b to
0e16ac2
Compare
jscheffl
reviewed
Jun 14, 2026
| clear_db_triggers() | ||
|
|
||
|
|
||
| def make_pool_stats( |
Contributor
There was a problem hiding this comment.
Would be nicer making this a real @fixture
jscheffl
approved these changes
Jun 14, 2026
jscheffl
left a comment
jscheffl
left a comment
Contributor
There was a problem hiding this comment.
Thanks for the break-up of the code and logic. That makes totally sense and due to the good pydoc makes it much more readable.
two nit from my side, then LGTM, hope we have this in 3.3.0? Would be cool as a clean start on a new minor.
ashb
commented
Jun 23, 2026
| max_tis: int, | ||
| pools: dict[str, PoolStats], | ||
| starved_pools: set[str], | ||
| session: Session, |
Member
Author
There was a problem hiding this comment.
Suggested change
| session: Session, | |
| *, | |
| session: Session, |
ashb
commented
Jun 23, 2026
| .limit(max_tis) | ||
| ) | ||
|
|
||
| def _mark_task_instances_queued(self, executable_tis: list[TI], session: Session) -> list[TI]: |
Member
Author
There was a problem hiding this comment.
Suggested change
| def _mark_task_instances_queued(self, executable_tis: list[TI], session: Session) -> list[TI]: | |
| def _mark_task_instances_queued(self, executable_tis: list[TI], *, session: Session) -> list[TI]: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I was looking at the core scheduler logic, and the
_executable_task_instances_to_queuedwas rather large, and hard for even me to understand, and past me wrote a good chunk of it!This splits it into two (plus a helper) focused methods to aid readability:
_acquire_pool_capacity: takes the advisory lock (lifetime of the session, so longer than just this fn) and reads pool utilisation via SELECT FOR UPDATE. Returns (pools, max_tis, starved_pools) so caller can short-circuit when all pools are full before doing any TI selection work._select_task_instances_to_queue: given pre-computed pool capacity, selects eligible SCHEDULED TIs and moves them to QUEUED. Accepts the pools dict and starved_pools set as parameters, making it directly testable without needing a real lock or DB pool read. This uses the new_build_schedulable_tis_queryhelper fn to build the complex query._critical_section_enqueue_task_instancesnow calls these two methods in sequence, making the two-phase structure (acquire capacity, then select and queue) more explicit.All test call sites updated to call
_select_task_instances_to_queuedirectly with amake_pool_stats()helper, removing the dependency on pool row locking in unit tests.No behaviour changes, just refactoring.