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

Record async migration execution time #615

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

fisehara
Copy link
Contributor

@fisehara fisehara commented Dec 13, 2022

Record async migration execution time

Record async migration execution time in migration status table.
Add migrationMetricsEmitter to emit metrics
Current metrics emitted:

  • async_migration_status
  • sync_migration_status

Change-type: minor
Signed-off-by: fisehara

@fisehara fisehara marked this pull request as draft December 13, 2022 14:37
auto-merge was automatically disabled December 13, 2022 14:37

Pull request was converted to draft

@fisehara fisehara force-pushed the record-async-migraiton-execution-time branch from a100f78 to 3e088bd Compare December 14, 2022 15:37
@fisehara fisehara requested a review from Page- December 14, 2022 15:37
@fisehara fisehara force-pushed the record-async-migraiton-execution-time branch from 3e088bd to 6758b37 Compare December 15, 2022 08:33
@fisehara fisehara marked this pull request as ready for review December 15, 2022 12:23
test/03-async-migrator.test.ts Outdated Show resolved Hide resolved
Comment on lines +163 to +164
ALTER TABLE "migration status"
ADD COLUMN IF NOT EXISTS "last execution time ms" INTEGER NULL;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This would also need to drop the start time column, and that potentially makes it a breaking change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Page-
I was unsure how to handle it. Basically PineJS doesn't know about old instances that reference the start time. My non favourite solution is to keep the column as is. My preferred solution would be a 2 staged deployment.

Basically it's not breaking the code it's breaking old-still running instances which require start time to be existing.
How was this handled in the past?

Record async migration execution time in migration status table.
Add migrationMetricsEmitter to emit metrics
Current metrics emitted:
- async_migration_status
- sync_migration_status

Change-type: minor
Signed-off-by: fisehara <[email protected]>
@fisehara fisehara force-pushed the record-async-migraiton-execution-time branch from 9fd1d70 to 04cded0 Compare December 20, 2022 20:09
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