fix(batch-exports): Couple of backfill bugfixes #25854
Conversation
posthog/batch_exports/models.py
Outdated
| if isinstance(end_at, str) and end_at.endswith("+00:00"): | ||
| end_at = end_at.replace("+00:00", "Z") |
There was a problem hiding this comment.
Python seems to prefer "+00:00" but Temporal schedules everything with "Z", it's quite annoying.
| @@ -173,7 +173,7 @@ async def backfill_schedule(inputs: BackfillScheduleInputs) -> None: | |||
| if details: | |||
| # If we receive details from a previous run, it means we were restarted for some reason. | |||
| # Let's not double-backfill and instead wait for any outstanding runs. | |||
| last_activity_details = HeartbeatDetails(*details[0]) | |||
| last_activity_details = HeartbeatDetails(*details) | |||
There was a problem hiding this comment.
details is already the tuple recorded by temporal. By doing details[0] we access the first item of the tuple (workflow_id), which is unpacked into a bunch of strings. This is, clearly, not what was intended. I think it's a remnant of the previous implementation that did things more manually instead of relying on Heartbeater.
tomasfarias
changed the title
posthog/batch_exports/models.py
Outdated
| end_at = self.end_at and self.end_at.isoformat() | ||
| start_at = self.start_at and self.start_at.isoformat() | ||
|
|
||
| if isinstance(end_at, str) and end_at.endswith("+00:00"): | ||
| end_at = end_at.replace("+00:00", "Z") |
There was a problem hiding this comment.
looks fine, but just wondering if there's there a reason you can't just do
end_at = self.end_at and self.end_at.strftime("%Y-%m-%dT%H:%M:%SZ")There was a problem hiding this comment.
I actually reverted this and copied the same formatting we use here: https://github.com/PostHog/posthog/blob/master/posthog/batch_exports/service.py#L475
Makes sense to have the service that starts the backfill and model be consistent. Although we could eventually update both. It's certainly annoying to remember to update both, so maybe some abstraction could be added.
Problem
A couple of bugs that need squashing:
We are not able to cancel backfills due to the Z suffix used by temporal instead of +00:00.Changes
👉 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?