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

fix: Fail backfill on schedule deletion #18575

Merged
merged 4 commits into from
Nov 14, 2023

Conversation

tomasfarias
Copy link
Contributor

@tomasfarias tomasfarias commented Nov 13, 2023

Problem

Backfills are not cleaned up properly when deleting an export. If a backfill is still running, it should stop if it's underlying schedule (batch export) is deleted.

Changes

  • Raise a non-retryable TemporalScheduleNotFoundError if we cannot get the schedule while waiting on the backfill runs.

👉 Stay up-to-date with PostHog coding conventions for a smoother review.

How did you test this code?

Added unit tests.

@tomasfarias tomasfarias marked this pull request as draft November 13, 2023 16:15
@tomasfarias tomasfarias force-pushed the fix/stop-backfill-on-schedule-delete branch 2 times, most recently from 5e8a7d2 to 3ec7050 Compare November 13, 2023 17:07
@tomasfarias tomasfarias force-pushed the fix/stop-backfill-on-schedule-delete branch from 3ec7050 to 80ad7b2 Compare November 13, 2023 17:09
@tomasfarias tomasfarias marked this pull request as ready for review November 13, 2023 17:10
@tomasfarias tomasfarias requested review from bretthoerner and a team and removed request for bretthoerner November 14, 2023 11:04
Copy link
Contributor

@bretthoerner bretthoerner left a comment

Choose a reason for hiding this comment

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

I like the concept, I wish the RPCError check could be tighter. It seems like it's actually gRPC, does that mean it always has a status code we could check?

I think so:

https://github.com/temporalio/sdk-python/blob/4e0883e61546b2cf3cb7d65557b6cf4efad2855c/temporalio/service.py#L785

https://github.com/temporalio/sdk-python/blob/4e0883e61546b2cf3cb7d65557b6cf4efad2855c/temporalio/service.py#L818

posthog/temporal/tests/utils/models.py Show resolved Hide resolved
posthog/temporal/workflows/backfill_batch_export.py Outdated Show resolved Hide resolved
@tomasfarias tomasfarias merged commit 9411f8e into master Nov 14, 2023
67 checks passed
@tomasfarias tomasfarias deleted the fix/stop-backfill-on-schedule-delete branch November 14, 2023 17:53
pauldambra pushed a commit that referenced this pull request Nov 15, 2023
* fix: Fail backfill on schedule deletion

* fix: Check first that a schedule exists

* fix: Do not retry on TemporalScheduleNotFoundError

* fix: Assume schedule deleted only with NOT_FOUND status
Copy link

sentry-io bot commented Nov 21, 2023

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

  • ‼️ TemporalScheduleNotFoundError: The Temporal Schedule 018bf3d2-9ddf-0001-ad9b-41be6e565046 was not found (maybe it was deleted?) posthog.temporal.workflows.backfill_batch_expor... View Issue

Did you find this useful? React with a 👍 or 👎

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