Skip to content
This repository was archived by the owner on Oct 9, 2023. It is now read-only.

Conversation

@eapolinario
Copy link
Contributor

@eapolinario eapolinario commented May 2, 2023

TL;DR

Migration to change parent_id type only if necessary

Type

  • Bug Fix
  • Feature
  • Plugin

Are all requirements met?

  • Code completed
  • Smoke tested
  • Unit tests added
  • Code documentation added
  • Any pending items have an associated Issue

Complete description

This blog post has a suggestion on how to run a postgres migration to change a column type from int to bigint without a lot of downtime.

I turned that into two gorm migrations. And tested extensively both on sandbox and in a real production table in RDS. The happy path works, but more testing is needed.

Tracking Issue

Remove the 'fixes' keyword if there will be multiple PRs to fix the linked issue

fixes https://github.com/flyteorg/flyte/issues/

Follow-up issue

NA
OR
https://github.com/flyteorg/flyte/issues/

Signed-off-by: eduardo apolinario <eapolinario@users.noreply.github.com>
@codecov
Copy link

codecov bot commented May 2, 2023

Codecov Report

Merging #554 (77e9241) into master (1026231) will increase coverage by 1.19%.
The diff coverage is 12.50%.

❗ Current head 77e9241 differs from pull request most recent head f28cc98. Consider uploading reports for the commit f28cc98 to get more accurate results

@@            Coverage Diff             @@
##           master     #554      +/-   ##
==========================================
+ Coverage   58.85%   60.05%   +1.19%     
==========================================
  Files         170      170              
  Lines       16068    13243    -2825     
==========================================
- Hits         9457     7953    -1504     
+ Misses       5777     4454    -1323     
- Partials      834      836       +2     
Flag Coverage Δ
unittests ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/repositories/config/migrations.go 2.50% <12.50%> (+1.36%) ⬆️

... and 155 files with indirect coverage changes

…ions` table

Signed-off-by: eduardo apolinario <eapolinario@users.noreply.github.com>
Signed-off-by: eduardo apolinario <eapolinario@users.noreply.github.com>
Signed-off-by: eduardo apolinario <eapolinario@users.noreply.github.com>
wild-endeavor
wild-endeavor previously approved these changes May 3, 2023
Signed-off-by: eduardo apolinario <eapolinario@users.noreply.github.com>
@eapolinario eapolinario merged commit 00915ec into master May 8, 2023
@eapolinario eapolinario deleted the special-case-migration--int-to-bigint branch May 8, 2023 17:33
wild-endeavor pushed a commit that referenced this pull request May 10, 2023
…ary (#554)

* Add migration to turn `parent_id` column into `bigint` only if necessary

Signed-off-by: eduardo apolinario <eapolinario@users.noreply.github.com>

* Migrations to handle the change of `parent_id` column in `node_executions` table

Signed-off-by: eduardo apolinario <eapolinario@users.noreply.github.com>

* Add unit test

Signed-off-by: eduardo apolinario <eapolinario@users.noreply.github.com>

* More coverage

Signed-off-by: eduardo apolinario <eapolinario@users.noreply.github.com>

* Create index outside transaction

Signed-off-by: eduardo apolinario <eapolinario@users.noreply.github.com>

---------

Signed-off-by: eduardo apolinario <eapolinario@users.noreply.github.com>
Co-authored-by: eduardo apolinario <eapolinario@users.noreply.github.com>
LaPetiteSouris pushed a commit to LaPetiteSouris/flyteadmin that referenced this pull request May 16, 2023
…ary (flyteorg#554)

* Add migration to turn `parent_id` column into `bigint` only if necessary

Signed-off-by: eduardo apolinario <eapolinario@users.noreply.github.com>

* Migrations to handle the change of `parent_id` column in `node_executions` table

Signed-off-by: eduardo apolinario <eapolinario@users.noreply.github.com>

* Add unit test

Signed-off-by: eduardo apolinario <eapolinario@users.noreply.github.com>

* More coverage

Signed-off-by: eduardo apolinario <eapolinario@users.noreply.github.com>

* Create index outside transaction

Signed-off-by: eduardo apolinario <eapolinario@users.noreply.github.com>

---------

Signed-off-by: eduardo apolinario <eapolinario@users.noreply.github.com>
Co-authored-by: eduardo apolinario <eapolinario@users.noreply.github.com>
Signed-off-by: TungHoang <sontunghoang@gmail.com>
eapolinario added a commit that referenced this pull request Sep 6, 2023
…ary (#554)

* Add migration to turn `parent_id` column into `bigint` only if necessary

Signed-off-by: eduardo apolinario <eapolinario@users.noreply.github.com>

* Migrations to handle the change of `parent_id` column in `node_executions` table

Signed-off-by: eduardo apolinario <eapolinario@users.noreply.github.com>

* Add unit test

Signed-off-by: eduardo apolinario <eapolinario@users.noreply.github.com>

* More coverage

Signed-off-by: eduardo apolinario <eapolinario@users.noreply.github.com>

* Create index outside transaction

Signed-off-by: eduardo apolinario <eapolinario@users.noreply.github.com>

---------

Signed-off-by: eduardo apolinario <eapolinario@users.noreply.github.com>
Co-authored-by: eduardo apolinario <eapolinario@users.noreply.github.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants