Skip to content
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

Reduce where clause fanout when updating workflow, node & task executions #5953

Merged

Conversation

katrogan
Copy link
Contributor

@katrogan katrogan commented Nov 4, 2024

Why are the changes needed?

In the executions path, performant updates are important since propeller sends executions events and doesn't proceed with the execution until the event is properly acked.

We've observed a performance increase (3x) for updating task executions in particular (which have 9!! primary keys) so that generated gorm adds a where clause for each primary key and this results in noticeably slower queries compared to updating on the unique BaseModel indexed Id field.

What changes were proposed in this pull request?

I'm only updating executions tables here (workflow, node and task) because they're in the hot path. Inventory like tasks and workflows can't be updated, launch plan state can but the perf requirements there are lower and the expected table size for static inventory is usually much lower than for executions

We can always assume the model has the id filled out because we fetch it before updating

How was this patch tested?

Tested on sandbox. Verified executions progressed and I could interact with the console. Verified dumped SQL showed updated query.

Setup process

Screenshots

Check all the applicable boxes

  • I updated the documentation accordingly.
  • All new and existing tests passed.
  • All commits are signed-off.

Related PRs

Docs link

@katrogan katrogan force-pushed the union/upstream-34c1276004c58416bc416dead07ebdd775e20410 branch from 5aa75fb to 3f2f3fe Compare November 4, 2024 12:51
Copy link

codecov bot commented Nov 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 36.81%. Comparing base (f745030) to head (3f2f3fe).
Report is 2 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #5953   +/-   ##
=======================================
  Coverage   36.81%   36.81%           
=======================================
  Files        1310     1310           
  Lines      131032   131035    +3     
=======================================
+ Hits        48234    48238    +4     
+ Misses      78612    78611    -1     
  Partials     4186     4186           
Flag Coverage Δ
unittests-datacatalog 51.58% <ø> (ø)
unittests-flyteadmin 54.05% <100.00%> (+<0.01%) ⬆️
unittests-flytecopilot 11.73% <ø> (ø)
unittests-flytectl 62.38% <ø> (ø)
unittests-flyteidl 6.92% <ø> (ø)
unittests-flyteplugins 53.64% <ø> (ø)
unittests-flytepropeller 42.90% <ø> (ø)
unittests-flytestdlib 55.41% <ø> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@katrogan katrogan merged commit a87585a into master Nov 5, 2024
51 checks passed
@katrogan katrogan deleted the union/upstream-34c1276004c58416bc416dead07ebdd775e20410 branch November 5, 2024 13:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants