From 5c75af4bee0b9bafc9eb253fdc48a28b7c0765d5 Mon Sep 17 00:00:00 2001 From: Jan Provaznik Date: Wed, 31 Jul 2019 16:08:42 +0200 Subject: Update epic discussion migration * added temporary index to make scheduling SQL query faster * update is done on DB side using MD5 function * refactoring of specs --- ...3142_migrate_discussion_id_on_promoted_epics.rb | 36 ++++++---- .../fix_promoted_epics_discussion_ids.rb | 32 +-------- .../fix_promoted_epics_discussion_ids_spec.rb | 46 +++++++++++++ .../migrate_promoted_epics_discussion_ids_spec.rb | 52 --------------- ...migrate_discussion_id_on_promoted_epics_spec.rb | 77 ++++++++++++++++++++++ 5 files changed, 148 insertions(+), 95 deletions(-) create mode 100644 spec/lib/gitlab/background_migration/fix_promoted_epics_discussion_ids_spec.rb delete mode 100644 spec/lib/gitlab/background_migration/migrate_promoted_epics_discussion_ids_spec.rb create mode 100644 spec/migrations/migrate_discussion_id_on_promoted_epics_spec.rb diff --git a/db/post_migrate/20190715193142_migrate_discussion_id_on_promoted_epics.rb b/db/post_migrate/20190715193142_migrate_discussion_id_on_promoted_epics.rb index 3fd68198f42..efafcee4723 100644 --- a/db/post_migrate/20190715193142_migrate_discussion_id_on_promoted_epics.rb +++ b/db/post_migrate/20190715193142_migrate_discussion_id_on_promoted_epics.rb @@ -13,34 +13,42 @@ class MigrateDiscussionIdOnPromotedEpics < ActiveRecord::Migration[5.2] disable_ddl_transaction! + class SystemNoteMtadata < ActiveRecord::Base + self.table_name = 'system_note_metadata' + end + class Note < ActiveRecord::Base include EachBatch + + has_one :system_note_metadata + self.table_name = 'notes' def self.fetch_discussion_ids_query promoted_epics_query = Note - .where(system: true) - .where(noteable_type: 'Epic') - .where("note LIKE 'promoted from%'") - .select("DISTINCT noteable_id") + .joins(:system_note_metadata) + .where(system: true) + .where(noteable_type: 'Epic') + .where("system_note_metadata.action='moved'") + .select("DISTINCT noteable_id") + Note.where(noteable_type: 'Epic') .where(noteable_id: promoted_epics_query) - .select("DISTINCT discussion_id").order(:discussion_id) + .order(:discussion_id) + .distinct.pluck(:discussion_id) end end def up - # add_concurrent_index(:system_note_metadata, :action) - - all_discussion_ids = Note.fetch_discussion_ids_query.collect(&:discussion_id) - index = 0 - all_discussion_ids.in_groups_of(BATCH_SIZE) do |ids| - index += 1 - delay = DELAY_INTERVAL * index - BackgroundMigrationWorker.perform_in(delay, MIGRATION, [ids.compact]) + add_concurrent_index(:system_note_metadata, :action, where: "action='moved'") + + all_discussion_ids = Note.fetch_discussion_ids_query + all_discussion_ids.in_groups_of(BATCH_SIZE, false).each_with_index do |ids, index| + delay = DELAY_INTERVAL * (index + 1) + BackgroundMigrationWorker.perform_in(delay, MIGRATION, [ids]) end - # add_concurrent_index(:system_note_metadata, :action) + remove_concurrent_index(:system_note_metadata, :action) end def down diff --git a/lib/gitlab/background_migration/fix_promoted_epics_discussion_ids.rb b/lib/gitlab/background_migration/fix_promoted_epics_discussion_ids.rb index 8c910f66fc8..77c317741e9 100644 --- a/lib/gitlab/background_migration/fix_promoted_epics_discussion_ids.rb +++ b/lib/gitlab/background_migration/fix_promoted_epics_discussion_ids.rb @@ -12,35 +12,9 @@ module Gitlab end def perform(discussion_ids) - discussion_values = build_discussion_values(discussion_ids) - - update_notes_discussion_ids(discussion_values) if discussion_values - end - - def build_discussion_values(discussion_ids) - discussion_ids.map do |discussion_id| - new_id = generate_id(discussion_id) - - ActiveRecord::Base.sanitize_sql(["(?, ?)", discussion_id, new_id]) - end.join(', ') - end - - def update_notes_discussion_ids(values) - sql = <<-SQL.squish - UPDATE notes SET discussion_id = v.new_discussion_id - FROM ( - VALUES - #{values} - ) AS v(old_discussion_id, new_discussion_id) - WHERE notes.discussion_id = v.old_discussion_id - AND notes.noteable_type = 'Epic' - SQL - - Note.connection.execute(sql) - end - - def generate_id(id) - Digest::SHA1.hexdigest([:discussion, 'epic', id, SecureRandom.hex].join("-")) + Note.where(noteable_type: 'Epic') + .where(discussion_id: discussion_ids) + .update_all("discussion_id=MD5(discussion_id)||substring(discussion_id from 1 for 8)") end end end diff --git a/spec/lib/gitlab/background_migration/fix_promoted_epics_discussion_ids_spec.rb b/spec/lib/gitlab/background_migration/fix_promoted_epics_discussion_ids_spec.rb new file mode 100644 index 00000000000..de25ca192af --- /dev/null +++ b/spec/lib/gitlab/background_migration/fix_promoted_epics_discussion_ids_spec.rb @@ -0,0 +1,46 @@ +# frozen_string_literal: true + +require 'rails_helper' + +describe Gitlab::BackgroundMigration::FixPromotedEpicsDiscussionIds, :migration, schema: 20190715193142 do + let(:namespaces) { table(:namespaces) } + let(:users) { table(:users) } + let(:epics) { table(:epics) } + let(:notes) { table(:notes) } + + let(:user) { users.create!(email: 'test@example.com', projects_limit: 100, username: 'test') } + let(:namespace) { namespaces.create(name: 'gitlab', path: 'gitlab-org') } + let(:epic1) { epics.create(id: 1, author_id: user.id, iid: 1, group_id: namespace.id, title: 'Epic with discussion', title_html: 'Epic with discussion') } + + def create_note(discussion_id) + notes.create(note: 'note comment', noteable_id: epic1.id, noteable_type: 'Epic', discussion_id: discussion_id) + end + + def expect_valid_discussion_id(id) + expect(id).to match(/[0-9a-f]{40}/) + end + + describe '#perform with batch of discussion ids' do + it 'updates discussion ids' do + note1 = create_note('00000000') + note2 = create_note('00000000') + note3 = create_note('10000000') + + subject.perform(%w(00000000 10000000)) + + expect_valid_discussion_id(note1.reload.discussion_id) + expect_valid_discussion_id(note2.reload.discussion_id) + expect_valid_discussion_id(note3.reload.discussion_id) + expect(note1.discussion_id).to eq(note2.discussion_id) + expect(note1.discussion_id).not_to eq(note3.discussion_id) + end + + it 'skips notes with discsussion id not in range' do + note4 = create_note('20000000') + + subject.perform(%w(00000000 10000000)) + + expect(note4.reload.discussion_id).to eq('20000000') + end + end +end diff --git a/spec/lib/gitlab/background_migration/migrate_promoted_epics_discussion_ids_spec.rb b/spec/lib/gitlab/background_migration/migrate_promoted_epics_discussion_ids_spec.rb deleted file mode 100644 index 16ca0e96ac6..00000000000 --- a/spec/lib/gitlab/background_migration/migrate_promoted_epics_discussion_ids_spec.rb +++ /dev/null @@ -1,52 +0,0 @@ -# frozen_string_literal: true - -require 'rails_helper' - -describe Gitlab::BackgroundMigration::FixPromotedEpicsDiscussionIds, :migration, schema: 20190715193142 do - let(:namespaces) { table(:namespaces) } - let(:projects) { table(:projects) } - let(:users) { table(:users) } - let(:issues) { table(:issues) } - let(:epics) { table(:epics) } - let(:notes) { table(:notes) } - - let(:user) { users.create!(email: 'test@example.com', projects_limit: 100, username: 'test') } - let(:namespace) { namespaces.create(name: 'gitlab', path: 'gitlab-org') } - let(:project) { projects.create(namespace_id: namespace.id, name: 'foo') } - let(:issue) { issues.create(project_id: project.id, title: 'Issue with discussion') } - let!(:issue_note) { notes.create(project_id: project.id, note: 'note comment', noteable_id: issue.id, noteable_type: 'Issue', discussion_id: 'd1') } - - describe '#perform with batch of discussion ids' do - let(:epic1) { epics.create(id: 1, author_id: user.id, iid: 1, group_id: namespace.id, title: 'Epic with discussion', title_html: 'Epic with discussion') } - let!(:epic1_note1) { notes.create(note: 'note comment', noteable_id: epic1.id, noteable_type: 'Epic', discussion_id: 'd1') } - let!(:epic1_note2) { notes.create(system: true, note: 'promoted from issue XXX', noteable_id: epic1.id, noteable_type: 'Epic', discussion_id: 'system1') } - - let(:epic2) { epics.create(id: 2, author_id: user.id, iid: 2, group_id: namespace.id, title: 'Epic with discussion', title_html: 'Epic with discussion') } - let!(:epic2_note1) { notes.create(note: 'note comment', noteable_id: epic2.id, noteable_type: 'Epic', discussion_id: 'd2') } - let!(:epic2_note2) { notes.create(system: true, note: 'promoted from issue YYY', noteable_id: epic2.id, noteable_type: 'Epic', discussion_id: 'system2') } - - it 'executes perform and changes discussion ids on all promoted epic discussions' do - expect(notes.where(discussion_id: 'd1').count).to eq(2) - - subject.perform(%w(d1 system1 d2 system2)) - - old_discussion_notes = notes.where(discussion_id: 'd1') - expect(old_discussion_notes.count).to eq(1) - expect(old_discussion_notes.first.noteable_type).to eq('Issue') - expect(epic1_note1.reload.discussion_id).not_to eq('d1') - expect(epic1_note2.reload.discussion_id).not_to eq('system1') - expect(epic2_note1.reload.discussion_id).not_to eq('d2') - expect(epic2_note2.reload.discussion_id).not_to eq('system2') - end - - it 'sets same new discussion id for all notes with the same old discussion id' do - note = notes.create(note: 'note comment', noteable_id: epic2.id, noteable_type: 'Epic', discussion_id: 'd2') - expect(notes.where(discussion_id: note.discussion_id).count).to eq(2) - - subject.perform(['d2']) - - expect(note.reload.discussion_id).not_to eq('d2') - expect(notes.where(discussion_id: note.discussion_id).count).to eq(2) - end - end -end diff --git a/spec/migrations/migrate_discussion_id_on_promoted_epics_spec.rb b/spec/migrations/migrate_discussion_id_on_promoted_epics_spec.rb new file mode 100644 index 00000000000..57c4bd16f2e --- /dev/null +++ b/spec/migrations/migrate_discussion_id_on_promoted_epics_spec.rb @@ -0,0 +1,77 @@ +# frozen_string_literal: true + +require 'spec_helper' +require Rails.root.join('db', 'post_migrate', '20190715193142_migrate_discussion_id_on_promoted_epics.rb') + +describe MigrateDiscussionIdOnPromotedEpics, :migration, :sidekiq do + let(:migration_class) { described_class::MIGRATION } + let(:migration_name) { migration_class.to_s.demodulize } + + let(:namespaces) { table(:namespaces) } + let(:projects) { table(:projects) } + let(:users) { table(:users) } + let(:issues) { table(:issues) } + let(:epics) { table(:epics) } + let(:notes) { table(:notes) } + let(:system_note_metadata) { table(:system_note_metadata) } + + let(:user) { users.create!(email: 'test@example.com', projects_limit: 100, username: 'test') } + let(:namespace) { namespaces.create(name: 'gitlab', path: 'gitlab-org') } + + def create_promotion_note(model, id) + note = notes.create!(system: true, note: 'promoted from issue XXX', + noteable_id: model.id, + noteable_type: model.class.name, + discussion_id: id) + system_note_metadata.create!(note_id: note.id, action: 'moved') + end + + def create_epic + epics.create!(author_id: user.id, iid: 1, + group_id: namespace.id, + title: 'Epic with discussion', + title_html: 'Epic with discussion') + end + + def create_note(model, id) + notes.create!(note: 'note', noteable_id: model.id, + noteable_type: model.class.name, discussion_id: id) + end + + context 'with promoted epic' do + let(:epic1) { create_epic } + let!(:note1) { create_promotion_note(epic1, 'id1') } + + it 'correctly schedules background migrations in batches' do + create_note(epic1, 'id2') + create_note(epic1, 'id3') + + stub_const("#{described_class.name}::BATCH_SIZE", 2) + + Sidekiq::Testing.fake! do + Timecop.freeze do + migrate! + + expect(migration_name).to be_scheduled_delayed_migration(2.minutes, %w(id1 id2)) + expect(migration_name).to be_scheduled_delayed_migration(4.minutes, %w(id3)) + expect(BackgroundMigrationWorker.jobs.size).to eq(2) + end + end + end + + it 'schedules only promoted epics' do + issue = issues.create(description: 'first', state: 'opened') + create_promotion_note(issue, 'id2') + create_note(create_epic, 'id3') + + Sidekiq::Testing.fake! do + Timecop.freeze do + migrate! + + expect(migration_name).to be_scheduled_delayed_migration(2.minutes, %w(id1)) + expect(BackgroundMigrationWorker.jobs.size).to eq(1) + end + end + end + end +end -- cgit v1.2.1