-
Notifications
You must be signed in to change notification settings - Fork 94
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
xtrigger efficiency fix. #5908
xtrigger efficiency fix. #5908
Conversation
Update the DB xtriggers table when the xtriggers get satisfied, not when the tasks that depend on them get satisfied.
ac07689
to
78f1618
Compare
Ready to go. Any reviewers available at your end @oliver-sanders ? (DS is still on leave here). |
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.
- Read code. Made a few comments, but none are critical to this PR.
- Manually tested bug and fix.
😄
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.
LGTM, though I'm not sure the DB table is doing what it's supposed to (before or after).
[unrelated] I had a go at running a workflow that calls an xtrigger which returns data ({'abc': 'def'}
). Taking a look at the result of running the workflow with --main-loop='log db'
, it looks like there are duplicate SQL calls updating the broadcast tables (before and after). Guessing this ain't meant to happen?
INSERT
OR REPLACE INTO broadcast_events
VALUES('2024-01-08T10:41:49.690744Z', '+', '20240108T0000Z', 'bar', '[environment]myxtrigger_abc', 'def')
INSERT
OR REPLACE INTO broadcast_events
VALUES('2024-01-08T10:41:49.692659Z', '+', '20240109T0000Z', 'bar', '[environment]myxtrigger_abc', 'def')
INSERT
OR REPLACE INTO broadcast_events
VALUES('2024-01-08T10:41:49.694808Z', '+', '20240110T0000Z', 'bar', '[environment]myxtrigger_abc', 'def')
INSERT
OR REPLACE INTO broadcast_events
VALUES('2024-01-08T10:41:49.697189Z', '+', '20240111T0000Z', 'bar', '[environment]myxtrigger_abc', 'def')
INSERT
OR REPLACE INTO broadcast_events
VALUES('2024-01-08T10:41:49.699815Z', '+', '20240112T0000Z', 'bar', '[environment]myxtrigger_abc', 'def')
INSERT
OR REPLACE INTO broadcast_states
VALUES('20240108T0000Z', 'bar', '[environment]myxtrigger_abc', 'def')
INSERT
OR REPLACE INTO broadcast_states
VALUES('20240109T0000Z', 'bar', '[environment]myxtrigger_abc', 'def')
INSERT
OR REPLACE INTO broadcast_states
VALUES('20240110T0000Z', 'bar', '[environment]myxtrigger_abc', 'def')
INSERT
OR REPLACE INTO broadcast_states
VALUES('20240111T0000Z', 'bar', '[environment]myxtrigger_abc', 'def')
INSERT
OR REPLACE INTO broadcast_states
VALUES('20240112T0000Z', 'bar', '[environment]myxtrigger_abc', 'def')
Co-authored-by: Oliver Sanders <[email protected]> Co-authored-by: Tim Pillinger <[email protected]>
bd40cd1
to
fe480c6
Compare
fe480c6
to
c43a644
Compare
Update the DB insert-maps for xtriggers when the xtriggers get satisfied, not when the tasks that depend on them get satisfied.
This bug (which causes duplicate entries in lists of DB updates) can cause long delays (minutes) just after initially populating the task pool, if hundreds of tasks depend on the same xtrigger (typically, a clock trigger).
The workflow that revealed this is large with several hundred cycles of runahead, resulting in 1.8 million update entries instead of ~800 🤯
We should get this into 8.2.4 given that it has caused problems out in the wild already.
Check List
CONTRIBUTING.md
and added my name as a Code Contributor.setup.cfg
(andconda-environment.yml
if present).CHANGES.md
entry included if this is a change that can affect users?.?.x
branch.