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.

Sign up for GitHub

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

Jump to bottom

Fix scheduling duplicate runs with expired idempotency keys #134

Merged
merged 7 commits into from
Nov 13, 2020
Merged

Fix scheduling duplicate runs with expired idempotency keys #134

merged 7 commits into from
Nov 13, 2020

Conversation

joshmeek
Copy link
Contributor

@joshmeek joshmeek commented Nov 12, 2020 •
edited
Loading

Summary

Through PrefectHQ/prefect#3629 we observed that there is potential for duplicate runs to be scheduled due to expired idempotency keys (24 hours). To patch this behavior this PR moves the last scheduled run check to the step prior to creating an idempotency key based on the parameter defaults and labels.

Importance

Duplicate runs are a hugely unexpected behavior and this patch is necessary.

Checklist

This PR:

  • adds new tests (if appropriate)
  • adds a change file in the changes/ directory (if appropriate)

@github-actions github-actions bot added the API label Nov 12, 2020
@joshmeek joshmeek requested a review from zanieb as a code owner November 12, 2020 21:15
zanieb
zanieb reviewed Nov 12, 2020
View reviewed changes
Copy link
Contributor

@zanieb zanieb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't weigh in on the implications of the TODO, only some minor comments

src/prefect_server/api/flows.py Outdated Show resolved Hide resolved
src/prefect_server/api/flows.py Outdated Show resolved Hide resolved
cicdw
cicdw requested changes Nov 13, 2020
View reviewed changes
Copy link
Member

@cicdw cicdw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Request to take this PR one step further

tests/api/test_flows.py Outdated Show resolved Hide resolved
@codecov-io
Copy link

Codecov Report

Merging #134 (9486dad) into master (ea53963) will decrease coverage by 0.16%.
The diff coverage is n/a.

cicdw
cicdw approved these changes Nov 13, 2020
View reviewed changes
Copy link
Member

@cicdw cicdw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome - this LGTM!

@cicdw cicdw merged commit 4fb6efa into master Nov 13, 2020
@cicdw cicdw deleted the dupe_fix branch November 13, 2020 17:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zanieb zanieb zanieb left review comments

@cicdw cicdw cicdw approved these changes

@jlowin jlowin Awaiting requested review from jlowin

Assignees
No one assigned
Labels
API
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@joshmeek @codecov-io @zanieb @cicdw