Merge branch 'mk/set-null-external-diff-store-to-local' into 'master'

Geo: Part 2 of Enforce not null merge_request_diffs.external_diff_store

See merge request gitlab-org/gitlab!38719

changelogs/unreleased/mk-set-null-external-diff-store-to-local.yml

+---
+title: Backfill null values to prepare for Geo replication feature
+merge_request: 38719
+author:
+type: added

db/post_migrate/20200805152108_migrate_null_external_diff_store_to_local_value.rb

+# frozen_string_literal: true
+
+class MigrateNullExternalDiffStoreToLocalValue < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+  JOB_INTERVAL = 2.minutes + 5.seconds
+  BATCH_SIZE = 5_000
+  MIGRATION = 'SetNullExternalDiffStoreToLocalValue'
+
+  disable_ddl_transaction!
+
+  class MergeRequestDiff < ActiveRecord::Base
+    self.table_name = 'merge_request_diffs'
+
+    include ::EachBatch
+  end
+
+  def up
+    # On GitLab.com, 19M of 93M rows have NULL external_diff_store.
+    #
+    # With batches of 5000 and a background migration job interval of 2m 5s,
+    # 3.8K jobs are scheduled over 5.5 days.
+    #
+    # The index `index_merge_request_diffs_external_diff_store_is_null` is
+    # expected to be used here and in the jobs.
+    #
+    # queue_background_migration_jobs_by_range_at_intervals is not used because
+    # it would enqueue 18.6K jobs and we have an index for getting these ranges.
+    MergeRequestDiff.where(external_diff_store: nil).each_batch(of: BATCH_SIZE) do |batch, index|
+      range = batch.pluck(Arel.sql("MIN(id)"), Arel.sql("MAX(id)")).first
+      delay = index * JOB_INTERVAL
+
+      migrate_in(delay.seconds, MIGRATION, [*range])
+    end
+  end
+
+  def down
+    # noop
+  end
+end

db/schema_migrations/20200805152108

+3976a084df88d7b6008a1d115d9c23d9c9084567c2cc5f1ddf68eaf1dc58ef3f
\ No newline at end of file

lib/gitlab/background_migration/set_null_external_diff_store_to_local_value.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module BackgroundMigration
+    # This class is responsible for migrating a range of merge request diffs
+    # with external_diff_store == NULL to 1.
+    #
+    # The index `index_merge_request_diffs_external_diff_store_is_null` is
+    # expected to be used to find the rows here and in the migration scheduling
+    # the jobs that run this class.
+    class SetNullExternalDiffStoreToLocalValue
+      LOCAL_STORE = 1 # equal to ObjectStorage::Store::LOCAL
+
+      # Temporary AR class for merge request diffs
+      class MergeRequestDiff < ActiveRecord::Base
+        self.table_name = 'merge_request_diffs'
+      end
+
+      def perform(start_id, stop_id)
+        MergeRequestDiff.where(external_diff_store: nil, id: start_id..stop_id).update_all(external_diff_store: LOCAL_STORE)
+      end
+    end
+  end
+end

spec/lib/gitlab/background_migration/set_null_external_diff_store_to_local_value_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+# The test setup must begin before
+# 20200804041930_add_not_null_constraint_on_external_diff_store_to_merge_request_diffs.rb
+# has run, or else we cannot insert a row with `NULL` `external_diff_store` to
+# test against.
+RSpec.describe Gitlab::BackgroundMigration::SetNullExternalDiffStoreToLocalValue, schema: 20200804035230 do
+  let!(:merge_request_diffs) { table(:merge_request_diffs) }
+  let!(:merge_requests)      { table(:merge_requests) }
+  let!(:namespaces)          { table(:namespaces) }
+  let!(:projects)            { table(:projects) }
+  let!(:namespace) { namespaces.create!(name: 'foo', path: 'foo') }
+  let!(:project) { projects.create!(namespace_id: namespace.id) }
+  let!(:merge_request) { merge_requests.create!(source_branch: 'x', target_branch: 'master', target_project_id: project.id) }
+
+  it 'correctly migrates nil external_diff_store to 1' do
+    external_diff_store_1 = merge_request_diffs.create!(external_diff_store: 1, merge_request_id: merge_request.id)
+    external_diff_store_2 = merge_request_diffs.create!(external_diff_store: 2, merge_request_id: merge_request.id)
+    external_diff_store_nil = merge_request_diffs.create!(external_diff_store: nil, merge_request_id: merge_request.id)
+
+    described_class.new.perform(external_diff_store_1.id, external_diff_store_nil.id)
+
+    external_diff_store_1.reload
+    external_diff_store_2.reload
+    external_diff_store_nil.reload
+
+    expect(external_diff_store_1.external_diff_store).to eq(1)   # unchanged
+    expect(external_diff_store_2.external_diff_store).to eq(2)   # unchanged
+    expect(external_diff_store_nil.external_diff_store).to eq(1) # nil => 1
+  end
+end