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

Migration for timezone aware timestamps #2587

Merged
merged 3 commits into from
May 28, 2024

Conversation

jedrazb
Copy link
Member

@jedrazb jedrazb commented May 28, 2024

Relates to #2067

Followup to #2067. I realised that migrating from naive to offset aware timestamps for job scheduling might break existing connectors that had scheduling enabled and were using naive datatime (no tz info) representation.

I tried this scenario, as expected after migrating to new offset aware logic the scheduling was broken ... I got a following error from a scheduler

  File "/Users/jedr/connectors-python/connectors/services/job_scheduling.py", line 207, in _should_schedule
    and last_sync_scheduled_at > last_wake_up_time
TypeError: can't compare offset-naive and offset-aware datetimes

The fix is to try to cast naive datetimes, to offset aware datetimes when reading last_access_control_sync_scheduled_at, last_incremental_sync_scheduled_at and last_sync_scheduled_at from ES index. This essentially takes care of the migration.

I modified _property_as_datetime function to try to set datetime to UTC timezone, with the with_utc_tz function.

The _property_as_datetime is used by following properties in protocol logic:

  • last_incremental_sync_scheduled_at this migration
  • last_access_control_sync_scheduled_at this migration
  • last_sync_scheduled_at this migration
  • last_seen - already indexed as offset aware timezone (no effect on logic)

Verification

  • Tested migration manually (enable all sync scheduling, execute some syncs, upgrade connectors, scheduling still works fine)
  • Unit tests

Checklists

Pre-Review Checklist

  • this PR has a meaningful title
  • this PR links to all relevant github issues that it fixes or partially addresses
  • if there is no GH issue, please create it. Each PR should have a link to an issue
  • this PR has a thorough description
  • Covered the changes with automated tests
  • Tested the changes locally
  • Added a label for each target release version (example: v7.13.2, v7.14.0, v8.0.0)

@@ -596,6 +597,8 @@ def _property_as_datetime(self, key):
value = self.get(key)
if value is not None:
value = parse_datetime_string(value) # pyright: ignore
# ensure that the datetime is in UTC timezone
Copy link
Member

Choose a reason for hiding this comment

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

I think it's also worth to add a comment here mentioning the problem found and saying that it's done here for backwards-compatibility

Copy link
Member Author

Choose a reason for hiding this comment

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

Okok, added the comment!

@jedrazb jedrazb merged commit 000b839 into main May 28, 2024
2 checks passed
@jedrazb jedrazb deleted the fix-timestamp-migration-between-naive-and-offset-aware branch May 28, 2024 09:30
Copy link

💔 Failed to create backport PR(s)

The backport operation could not be completed due to the following error:
There are no branches to backport to. Aborting.

The backport PRs will be merged automatically after passing CI.

To backport manually run:
backport --pr 2587 --autoMerge --autoMergeMethod squash

@artem-shelkovnikov
Copy link
Member

@jedrazb any objections to backporting it back up to 8.13?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants