diff --git a/db/migrate/20190211131150_add_state_id_to_issuables.rb b/db/migrate/20190211131150_add_state_id_to_issuables.rb index af02aa84afd..b9d52fe63cd 100644 --- a/db/migrate/20190211131150_add_state_id_to_issuables.rb +++ b/db/migrate/20190211131150_add_state_id_to_issuables.rb @@ -1,5 +1,6 @@ class AddStateIdToIssuables < ActiveRecord::Migration[5.0] include Gitlab::Database::MigrationHelpers + #include AfterCommitQueue DOWNTIME = false MIGRATION = 'SyncIssuablesStateId'.freeze @@ -26,8 +27,13 @@ class AddStateIdToIssuables < ActiveRecord::Migration[5.0] add_column :issues, :state_id, :integer, limit: 1 add_column :merge_requests, :state_id, :integer, limit: 1 - queue_background_migration_jobs_by_range_at_intervals(Issue.where(state_id: nil), MIGRATION, DELAY_INTERVAL, batch_size: BATCH_SIZE) - queue_background_migration_jobs_by_range_at_intervals(MergeRequest.where(state_id: nil), MIGRATION, DELAY_INTERVAL, batch_size: BATCH_SIZE) + # Is this safe? + # Added to avoid an warning about jobs running inside transactions. + # Since we only add a column this should be ok + Sidekiq::Worker.skipping_transaction_check do + queue_background_migration_jobs_by_range_at_intervals(Issue.where(state_id: nil), MIGRATION, DELAY_INTERVAL, batch_size: BATCH_SIZE) + queue_background_migration_jobs_by_range_at_intervals(MergeRequest.where(state_id: nil), MIGRATION, DELAY_INTERVAL, batch_size: BATCH_SIZE) + end end def down diff --git a/lib/gitlab/background_migration/sync_issuables_state_id.rb b/lib/gitlab/background_migration/sync_issuables_state_id.rb index 1ac86b8acf2..95734a7310e 100644 --- a/lib/gitlab/background_migration/sync_issuables_state_id.rb +++ b/lib/gitlab/background_migration/sync_issuables_state_id.rb @@ -4,15 +4,15 @@ module Gitlab module BackgroundMigration class SyncIssuablesStateId - def perform(start_id, end_id, model_class) - populate_new_state_id(start_id, end_id, model_class) + def perform(start_id, end_id, table_name) + populate_new_state_id(start_id, end_id, table_name) end - def populate_new_state_id(start_id, end_id, model_class) - Rails.logger.info("#{model_class.model_name.human} - Populating state_id: #{start_id} - #{end_id}") + def populate_new_state_id(start_id, end_id, table_name) + Rails.logger.info("#{table_name} - Populating state_id: #{start_id} - #{end_id}") ActiveRecord::Base.connection.execute <<~SQL - UPDATE #{model_class.table_name} + UPDATE #{table_name} SET state_id = CASE state WHEN 'opened' THEN 1 diff --git a/lib/gitlab/database/migration_helpers.rb b/lib/gitlab/database/migration_helpers.rb index 20cbb9e096b..46b36d07c20 100644 --- a/lib/gitlab/database/migration_helpers.rb +++ b/lib/gitlab/database/migration_helpers.rb @@ -1029,11 +1029,12 @@ into similar problems in the future (e.g. when new tables are created). model_class.each_batch(of: batch_size) do |relation, index| start_id, end_id = relation.pluck('MIN(id), MAX(id)').first + table_name = relation.base_class.table_name # `BackgroundMigrationWorker.bulk_perform_in` schedules all jobs for # the same time, which is not helpful in most cases where we wish to # spread the work over time. - BackgroundMigrationWorker.perform_in(delay_interval * index, job_class_name, [start_id, end_id, model_class]) + BackgroundMigrationWorker.perform_in(delay_interval * index, job_class_name, [start_id, end_id, table_name]) end end diff --git a/spec/migrations/add_state_id_to_issuables_spec.rb b/spec/migrations/add_state_id_to_issuables_spec.rb index 4416f416c18..b0e285db1f3 100644 --- a/spec/migrations/add_state_id_to_issuables_spec.rb +++ b/spec/migrations/add_state_id_to_issuables_spec.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true require 'spec_helper' -require Rails.root.join('db', 'migrate', '20190206144959_change_issuable_states_to_integer.rb') +require Rails.root.join('db', 'migrate', '20190211131150_add_state_id_to_issuables.rb') describe AddStateIdToIssuables, :migration do let(:namespaces) { table(:namespaces) } @@ -20,14 +20,15 @@ describe AddStateIdToIssuables, :migration do it 'migrates state column to integer' do opened_issue = issues.create!(description: 'first', state: 'opened') closed_issue = issues.create!(description: 'second', state: 'closed') + invalid_state_issue = issues.create!(description: 'fourth', state: 'not valid') nil_state_issue = issues.create!(description: 'third', state: nil) migrate! - issues.reset_column_information - expect(opened_issue.reload.state).to eq(Issue.states.opened) - expect(closed_issue.reload.state).to eq(Issue.states.closed) - expect(nil_state_issue.reload.state).to eq(nil) + expect(opened_issue.reload.state_id).to eq(Issue.states.opened) + expect(closed_issue.reload.state_id).to eq(Issue.states.closed) + expect(invalid_state_issue.reload.state_id).to be_nil + expect(nil_state_issue.reload.state_id).to be_nil end end @@ -37,52 +38,15 @@ describe AddStateIdToIssuables, :migration do closed_merge_request = merge_requests.create!(state: 'closed', target_project_id: @project.id, target_branch: 'feature2', source_branch: 'master') merged_merge_request = merge_requests.create!(state: 'merged', target_project_id: @project.id, target_branch: 'feature3', source_branch: 'master') locked_merge_request = merge_requests.create!(state: 'locked', target_project_id: @project.id, target_branch: 'feature4', source_branch: 'master') - nil_state_merge_request = merge_requests.create!(state: nil, target_project_id: @project.id, target_branch: 'feature5', source_branch: 'master') + invalid_state_merge_request = merge_requests.create!(state: 'not valid', target_project_id: @project.id, target_branch: 'feature5', source_branch: 'master') migrate! - merge_requests.reset_column_information - expect(opened_merge_request.reload.state).to eq(MergeRequest.states.opened) - expect(closed_merge_request.reload.state).to eq(MergeRequest.states.closed) - expect(merged_merge_request.reload.state).to eq(MergeRequest.states.merged) - expect(locked_merge_request.reload.state).to eq(MergeRequest.states.locked) - expect(nil_state_merge_request.reload.state).to eq(nil) - end - end - end - - describe '#down' do - context 'issues' do - it 'migrates state column to string' do - opened_issue = issues.create!(description: 'first', state: 1) - closed_issue = issues.create!(description: 'second', state: 2) - nil_state_issue = issues.create!(description: 'third', state: nil) - - migration.down - - issues.reset_column_information - expect(opened_issue.reload.state).to eq('opened') - expect(closed_issue.reload.state).to eq('closed') - expect(nil_state_issue.reload.state).to eq(nil) - end - end - - context 'merge requests' do - it 'migrates state column to string' do - opened_merge_request = merge_requests.create!(state: 1, target_project_id: @project.id, target_branch: 'feature1', source_branch: 'master') - closed_merge_request = merge_requests.create!(state: 2, target_project_id: @project.id, target_branch: 'feature2', source_branch: 'master') - merged_merge_request = merge_requests.create!(state: 3, target_project_id: @project.id, target_branch: 'feature3', source_branch: 'master') - locked_merge_request = merge_requests.create!(state: 4, target_project_id: @project.id, target_branch: 'feature4', source_branch: 'master') - nil_state_merge_request = merge_requests.create!(state: nil, target_project_id: @project.id, target_branch: 'feature5', source_branch: 'master') - - migration.down - - merge_requests.reset_column_information - expect(opened_merge_request.reload.state).to eq('opened') - expect(closed_merge_request.reload.state).to eq('closed') - expect(merged_merge_request.reload.state).to eq('merged') - expect(locked_merge_request.reload.state).to eq('locked') - expect(nil_state_merge_request.reload.state).to eq(nil) + expect(opened_merge_request.reload.state_id).to eq(MergeRequest.states.opened) + expect(closed_merge_request.reload.state_id).to eq(MergeRequest.states.closed) + expect(merged_merge_request.reload.state_id).to eq(MergeRequest.states.merged) + expect(locked_merge_request.reload.state_id).to eq(MergeRequest.states.locked) + expect(invalid_state_merge_request.reload.state_id).to be_nil end end end