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

feat: Use a background task to set batch export status to running #23136

Merged

Conversation

tomasfarias
Copy link
Contributor

Problem

We are not reliably setting the status of a batch export run to running. We timeout regularly while waiting for it to be done.

Changes

This allows us to run the task concurrently instead of timing out on it. It also has the additional benefit of removing a lot of nesting levels!

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

Does this work well for both Cloud and self-hosted?

How did you test this code?

Added unit test.

@tomasfarias
Copy link
Contributor Author

The large diff lines are just the nesting being removed, actual changes are small!

@tomasfarias tomasfarias force-pushed the feat/use-background-task-to-set-batch-export-to-running branch from 62f95a8 to 4ec5623 Compare June 21, 2024 12:23
@tomasfarias tomasfarias marked this pull request as ready for review June 21, 2024 12:24
@tomasfarias tomasfarias requested review from EDsCODE and Gilbert09 June 21, 2024 12:55
@tomasfarias tomasfarias force-pushed the feat/use-background-task-to-set-batch-export-to-running branch 2 times, most recently from aed05e0 to 9034a2c Compare June 25, 2024 09:02
@tomasfarias tomasfarias changed the base branch from master to feat/person-batch-exports-big-query June 26, 2024 15:03
Base automatically changed from feat/person-batch-exports-big-query to master June 27, 2024 10:10
@tomasfarias tomasfarias force-pushed the feat/use-background-task-to-set-batch-export-to-running branch 2 times, most recently from aef5969 to 1285e61 Compare June 27, 2024 13:46
@tomasfarias tomasfarias force-pushed the feat/use-background-task-to-set-batch-export-to-running branch from 230bb29 to c10ff4d Compare June 28, 2024 08:09
@tomasfarias tomasfarias merged commit fc80f93 into master Jun 28, 2024
84 checks passed
@tomasfarias tomasfarias deleted the feat/use-background-task-to-set-batch-export-to-running branch June 28, 2024 11:41
Copy link

sentry-io bot commented Jun 28, 2024

Suspect Issues

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

  • ‼️ Forbidden: DELETE https://bigquery.googleapis.com/bigquery/v2/projects/triple-whale-ops/datasets/posthog_nat... posthog.temporal.batch_exports.bigquery_batch_e... View Issue
  • ‼️ ClickHouseError: Code: 47. DB::Exception: Missing columns: 'events.uuid' 'events.person_id' 'events.timestamp' 'ev... posthog.temporal.common.clickhouse in check_res... View Issue
  • ‼️ OSError: Expected to be able to read 7108328 bytes for message body, got 5258384 pyarrow.lib in pyarrow.lib.check_status View Issue
  • ‼️ ConnectionTimeout: connection timeout expired posthog.temporal.batch_exports.postgres_batch_e... View Issue
  • ‼️ OSError: Expected to be able to read 5753808 bytes for message body, got 2276496 pyarrow.lib in pyarrow.lib.check_status View Issue

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

Phanatic pushed a commit that referenced this pull request Jul 8, 2024
…3136)

* feat: Use a background task to set batch export status to running

This allows us to run the task concurrently instead of timing out on
it. It also has the additional benefit of removing a lot of nesting levels!

* fix: Type hints and early return

* fix: Update type hint

* fix: Merge conflicts

* fix: Rebase

* fix: Bunch of merge conflicts

* Update query snapshots

* test: Add unit test

* Update query snapshots

* chore: Rename file

* fix: Use new 3.11 alias

* Update query snapshots

* Update query snapshots

* Update query snapshots

* Update query snapshots

* Update query snapshots

* fix: Use new 3.11 alias

* Update query snapshots

---------

Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
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