diff options
author | Felipe Artur <fcardozo@gitlab.com> | 2019-05-06 22:45:17 +0300 |
---|---|---|
committer | Sean McGivern <sean@gitlab.com> | 2019-05-06 22:45:17 +0300 |
commit | c40bad741f963f0b3bf3868d485e419dbbf7b37c (patch) | |
tree | e41ea99f73a250154b8923eb2cca0ca1ce57ad61 | |
parent | adc83d25a7ebf636941f72881a8828e6ebb5ad64 (diff) |
Fix issuables state_id nil when importing projects from GitHub
Issues and merge requests imported from GitHub are having state_id
set to null. This fixes the GitHub project importer and schedule
migrations to fix state_id.
11 files changed, 169 insertions, 1 deletions
diff --git a/changelogs/unreleased/issue_57906_fix_github_import.yml b/changelogs/unreleased/issue_57906_fix_github_import.yml new file mode 100644 index 00000000000..d28a78d5d11 --- /dev/null +++ b/changelogs/unreleased/issue_57906_fix_github_import.yml @@ -0,0 +1,5 @@ +--- +title: Fix issuables state_id nil when importing projects from GitHub +merge_request: 28027 +author: +type: fixed diff --git a/db/migrate/20190506135337_add_temporary_indexes_to_state_id.rb b/db/migrate/20190506135337_add_temporary_indexes_to_state_id.rb new file mode 100644 index 00000000000..8e9838e1afb --- /dev/null +++ b/db/migrate/20190506135337_add_temporary_indexes_to_state_id.rb @@ -0,0 +1,34 @@ +# frozen_string_literal: true + +# This migration adds temporary indexes to state_id column of issues +# and merge_requests tables. It will be used only to peform the scheduling +# for populating state_id in a post migrate and will be removed after it. +# Check: ScheduleSyncIssuablesStateIdWhereNil. + +class AddTemporaryIndexesToStateId < ActiveRecord::Migration[5.1] + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + disable_ddl_transaction! + + def up + %w(issues merge_requests).each do |table| + add_concurrent_index( + table, + 'id', + name: index_name_for(table), + where: "state_id IS NULL" + ) + end + end + + def down + remove_concurrent_index_by_name(:issues, index_name_for("issues")) + remove_concurrent_index_by_name(:merge_requests, index_name_for("merge_requests")) + end + + def index_name_for(table) + "idx_on_#{table}_where_state_id_is_null" + end +end diff --git a/db/post_migrate/20190506135400_schedule_sync_issuables_state_id_where_nil.rb b/db/post_migrate/20190506135400_schedule_sync_issuables_state_id_where_nil.rb new file mode 100644 index 00000000000..4c31b5968ff --- /dev/null +++ b/db/post_migrate/20190506135400_schedule_sync_issuables_state_id_where_nil.rb @@ -0,0 +1,63 @@ +# frozen_string_literal: true + +class ScheduleSyncIssuablesStateIdWhereNil < ActiveRecord::Migration[5.1] + # Issues and MergeRequests imported by GitHub are being created with + # state_id = null, this fixes them. + # + # Part of a bigger plan: https://gitlab.com/gitlab-org/gitlab-ce/issues/51789 + + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + # 2019-05-02 gitlab.com issuable numbers + # issues with state_id nil: ~40000 + # merge requests with state_id nil: ~200000 + # + # Using 5000 as batch size and 120 seconds interval will create: + # ~8 jobs for issues - taking ~16 minutes + # ~40 jobs for merge requests - taking ~1.34 hours + # + BATCH_SIZE = 5000 + DELAY_INTERVAL = 120.seconds.to_i + ISSUES_MIGRATION = 'SyncIssuesStateId'.freeze + MERGE_REQUESTS_MIGRATION = 'SyncMergeRequestsStateId'.freeze + + disable_ddl_transaction! + + class Issue < ActiveRecord::Base + include EachBatch + + self.table_name = 'issues' + end + + class MergeRequest < ActiveRecord::Base + include EachBatch + + self.table_name = 'merge_requests' + end + + def up + queue_background_migration_jobs_by_range_at_intervals( + Issue.where(state_id: nil), + ISSUES_MIGRATION, + DELAY_INTERVAL, + batch_size: BATCH_SIZE + ) + + queue_background_migration_jobs_by_range_at_intervals( + MergeRequest.where(state_id: nil), + MERGE_REQUESTS_MIGRATION, + DELAY_INTERVAL, + batch_size: BATCH_SIZE + ) + + # Remove temporary indexes added on "AddTemporaryIndexesToStateId" + remove_concurrent_index_by_name(:issues, "idx_on_issues_where_state_id_is_null") + remove_concurrent_index_by_name(:merge_requests, "idx_on_merge_requests_where_state_id_is_null") + end + + def down + # No op + end +end diff --git a/db/schema.rb b/db/schema.rb index 1bcd22dc81c..deaf406fe3d 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20190426180107) do +ActiveRecord::Schema.define(version: 20190506135400) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" diff --git a/lib/gitlab/bitbucket_server_import/importer.rb b/lib/gitlab/bitbucket_server_import/importer.rb index 1d3ddeeb0f1..ff2694abd5e 100644 --- a/lib/gitlab/bitbucket_server_import/importer.rb +++ b/lib/gitlab/bitbucket_server_import/importer.rb @@ -201,6 +201,7 @@ module Gitlab target_branch: Gitlab::Git.ref_name(pull_request.target_branch_name), target_branch_sha: pull_request.target_branch_sha, state: pull_request.state, + state_id: MergeRequest.available_states[pull_request.state], author_id: author_id, assignee_id: nil, created_at: pull_request.created_at, diff --git a/lib/gitlab/github_import/importer/issue_importer.rb b/lib/gitlab/github_import/importer/issue_importer.rb index 656d46b6a7d..a468f6d8821 100644 --- a/lib/gitlab/github_import/importer/issue_importer.rb +++ b/lib/gitlab/github_import/importer/issue_importer.rb @@ -53,6 +53,7 @@ module Gitlab description: description, milestone_id: milestone_finder.id_for(issue), state: issue.state, + state_id: ::Issue.available_states[issue.state], created_at: issue.created_at, updated_at: issue.updated_at } diff --git a/lib/gitlab/github_import/importer/pull_request_importer.rb b/lib/gitlab/github_import/importer/pull_request_importer.rb index 1b293ddc7c7..377e873d24d 100644 --- a/lib/gitlab/github_import/importer/pull_request_importer.rb +++ b/lib/gitlab/github_import/importer/pull_request_importer.rb @@ -55,6 +55,7 @@ module Gitlab source_branch: pull_request.formatted_source_branch, target_branch: pull_request.target_branch, state: pull_request.state, + state_id: ::MergeRequest.available_states[pull_request.state], milestone_id: milestone_finder.id_for(pull_request), author_id: author_id, assignee_id: user_finder.assignee_id_for(pull_request), diff --git a/spec/lib/gitlab/bitbucket_server_import/importer_spec.rb b/spec/lib/gitlab/bitbucket_server_import/importer_spec.rb index 1e90a2ef27f..cc09804fd53 100644 --- a/spec/lib/gitlab/bitbucket_server_import/importer_spec.rb +++ b/spec/lib/gitlab/bitbucket_server_import/importer_spec.rb @@ -105,6 +105,7 @@ describe Gitlab::BitbucketServerImport::Importer do expect(merge_request.metrics.merged_by).to eq(project.owner) expect(merge_request.metrics.merged_at).to eq(@merge_event.merge_timestamp) expect(merge_request.merge_commit_sha).to eq('12345678') + expect(merge_request.state_id).to eq(3) end it 'imports comments' do diff --git a/spec/lib/gitlab/github_import/importer/issue_importer_spec.rb b/spec/lib/gitlab/github_import/importer/issue_importer_spec.rb index 7901ae005d9..dab5767ece1 100644 --- a/spec/lib/gitlab/github_import/importer/issue_importer_spec.rb +++ b/spec/lib/gitlab/github_import/importer/issue_importer_spec.rb @@ -98,6 +98,7 @@ describe Gitlab::GithubImport::Importer::IssueImporter, :clean_gitlab_redis_cach description: 'This is my issue', milestone_id: milestone.id, state: :opened, + state_id: 1, created_at: created_at, updated_at: updated_at }, @@ -127,6 +128,7 @@ describe Gitlab::GithubImport::Importer::IssueImporter, :clean_gitlab_redis_cach description: "*Created by: alice*\n\nThis is my issue", milestone_id: milestone.id, state: :opened, + state_id: 1, created_at: created_at, updated_at: updated_at }, diff --git a/spec/lib/gitlab/github_import/importer/pull_request_importer_spec.rb b/spec/lib/gitlab/github_import/importer/pull_request_importer_spec.rb index 2e4a7c36fb8..6d614c6527a 100644 --- a/spec/lib/gitlab/github_import/importer/pull_request_importer_spec.rb +++ b/spec/lib/gitlab/github_import/importer/pull_request_importer_spec.rb @@ -93,6 +93,7 @@ describe Gitlab::GithubImport::Importer::PullRequestImporter, :clean_gitlab_redi source_branch: 'github/fork/alice/feature', target_branch: 'master', state: :merged, + state_id: 3, milestone_id: milestone.id, author_id: user.id, assignee_id: user.id, @@ -138,6 +139,7 @@ describe Gitlab::GithubImport::Importer::PullRequestImporter, :clean_gitlab_redi source_branch: 'github/fork/alice/feature', target_branch: 'master', state: :merged, + state_id: 3, milestone_id: milestone.id, author_id: project.creator_id, assignee_id: user.id, @@ -184,6 +186,7 @@ describe Gitlab::GithubImport::Importer::PullRequestImporter, :clean_gitlab_redi source_branch: 'master-42', target_branch: 'master', state: :merged, + state_id: 3, milestone_id: milestone.id, author_id: user.id, assignee_id: user.id, diff --git a/spec/migrations/schedule_sync_issuables_state_id_where_nil_spec.rb b/spec/migrations/schedule_sync_issuables_state_id_where_nil_spec.rb new file mode 100644 index 00000000000..105c05bb7ca --- /dev/null +++ b/spec/migrations/schedule_sync_issuables_state_id_where_nil_spec.rb @@ -0,0 +1,57 @@ +# frozen_string_literal: true + +require 'spec_helper' +require Rails.root.join('db', 'post_migrate', '20190506135400_schedule_sync_issuables_state_id_where_nil') + +describe ScheduleSyncIssuablesStateIdWhereNil, :migration, :sidekiq do + let(:namespaces) { table(:namespaces) } + let(:projects) { table(:projects) } + let(:merge_requests) { table(:merge_requests) } + let(:issues) { table(:issues) } + let(:migration) { described_class.new } + let(:group) { namespaces.create!(name: 'gitlab', path: 'gitlab') } + let(:project) { projects.create!(namespace_id: group.id) } + + shared_examples 'scheduling migrations' do + before do + Sidekiq::Worker.clear_all + stub_const("#{described_class.name}::BATCH_SIZE", 2) + end + + it 'correctly schedules issuable sync background migration' do + Sidekiq::Testing.fake! do + Timecop.freeze do + migrate! + + expect(migration).to be_scheduled_delayed_migration(120.seconds, resource_1.id, resource_3.id) + expect(migration).to be_scheduled_delayed_migration(240.seconds, resource_5.id, resource_5.id) + expect(BackgroundMigrationWorker.jobs.size).to eq(2) + end + end + end + end + + describe '#up' do + context 'issues' do + it_behaves_like 'scheduling migrations' do + let(:migration) { described_class::ISSUES_MIGRATION } + let!(:resource_1) { issues.create!(description: 'first', state: 'opened', state_id: nil) } + let!(:resource_2) { issues.create!(description: 'second', state: 'closed', state_id: 2) } + let!(:resource_3) { issues.create!(description: 'third', state: 'closed', state_id: nil) } + let!(:resource_4) { issues.create!(description: 'fourth', state: 'closed', state_id: 2) } + let!(:resource_5) { issues.create!(description: 'fifth', state: 'closed', state_id: nil) } + end + end + + context 'merge requests' do + it_behaves_like 'scheduling migrations' do + let(:migration) { described_class::MERGE_REQUESTS_MIGRATION } + let!(:resource_1) { merge_requests.create!(state: 'opened', state_id: nil, target_project_id: project.id, target_branch: 'feature1', source_branch: 'master') } + let!(:resource_2) { merge_requests.create!(state: 'closed', state_id: 2, target_project_id: project.id, target_branch: 'feature2', source_branch: 'master') } + let!(:resource_3) { merge_requests.create!(state: 'merged', state_id: nil, target_project_id: project.id, target_branch: 'feature3', source_branch: 'master') } + let!(:resource_4) { merge_requests.create!(state: 'locked', state_id: 3, target_project_id: project.id, target_branch: 'feature4', source_branch: 'master') } + let!(:resource_5) { merge_requests.create!(state: 'locked', state_id: nil, target_project_id: project.id, target_branch: 'feature4', source_branch: 'master') } + end + end + end +end |