Fix #39: mirror an unchanged poll status with one conditional update - #93
Open
corgab wants to merge 2 commits into
Open
Fix #39: mirror an unchanged poll status with one conditional update#93corgab wants to merge 2 commits into
corgab wants to merge 2 commits into
Conversation
Every non-terminal poll read the quantum_tasks row and saved it back; Eloquent already skipped the UPDATE when nothing changed, but the SELECT ran on each of the up to 720 polls of a queued task. A status-only update is now a single conditional UPDATE keyed on the task ARN and the previous status, so an unchanged poll costs one query and writes no row, and updated_at only moves when the status does. Terminal transitions keep the model path, which also records counts, errors and timestamps. Closes #39
…rs skip model events Builder::update() already adds updated_at, so the explicit value only tied the job to the column name. The docblock and README now say that the status-only update bypasses the QuantumTask model events, while terminal transitions still fire them.
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
1 participant
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.
Summary
PollQuantumTask::persist()ran on every non-terminal poll: aSELECTof thequantum_tasksrow followed bysave(). Measured onmainwith the query log, the second poll of an unchangedRUNNINGtask issues exactly oneSELECTand noUPDATE: Eloquent already skips the write when nothing is dirty, so the amplification the issue describes is one read per poll rather than one write. That is still up to 720 avoidable queries per task at the default polling budget.Plan
Make the intermediate-status path a single conditional
UPDATEkeyed on the task ARN and on the status being different, so an unchanged poll is one statement that touches no row and moves noupdated_at, and a changed status is written without reading the row first. Keep the model path for terminal transitions, which also record counts, errors and timestamps.Changes
PollQuantumTask::persist(): when neither counts nor an error are given,QuantumTask::query()->where('task_arn', ...)->where('status', '!=', $status->value)->update(['status' => ...])(Eloquent stampsupdated_atitself); otherwise the existing load-and-save path.QuantumTaskmodel observers no longer seeCREATED/QUEUED/RUNNINGchanges, only the terminal transition. Docblock and README say so and point atCircuitCompletedor the table for earlier states. Keeping those events would require the per-pollSELECTthis PR removes.No config change, no new dependencies, no change to what ends up in the table.
Tests
PersistenceTest: a second poll with the same status runs exactly one query, it is anUPDATE, andupdated_atdoes not move (time is advanced between the polls to prove it); a status change fromQUEUEDtoRUNNINGis recorded with a single query and no precedingSELECT. The first test fails onmain(the query is aSELECT), and the existing intermediate-status and terminal-state tests keep passing.Local: pint passed, pest 793 tests passed.
Closes #39