Skip to content
Snippets Groups Projects
Verified Commit e56b5636 authored by Michał Zając's avatar Michał Zając Committed by GitLab
Browse files

Merge branch 'pedropombeiro/459999/allow-configuring-batch_column' into 'master'

Allow customizing batch_column in shared examples

See merge request !164439



Merged-by: default avatarMichał Zając <mzajac@gitlab.com>
Approved-by: default avatarMax Orefice <morefice@gitlab.com>
Approved-by: Rajendra Kadam's avatarRajendra Kadam <rkadam@gitlab.com>
Approved-by: default avatarMichał Zając <mzajac@gitlab.com>
Reviewed-by: default avatarPedro Pombeiro <noreply@pedro.pombei.ro>
Reviewed-by: default avatarMax Orefice <morefice@gitlab.com>
Reviewed-by: Rajendra Kadam's avatarRajendra Kadam <rkadam@gitlab.com>
Co-authored-by: default avatarPedro Pombeiro <noreply@pedro.pombei.ro>
parents 9ed7a62c 0fe365f2
No related branches found
No related tags found
1 merge request!164439Allow customizing batch_column in shared examples
Pipeline #1437595489 canceled
......@@ -20,7 +20,7 @@ def construct_query(sub_batch:)
SET #{backfill_column} = #{backfill_via_table}.#{backfill_via_column}
FROM #{backfill_via_table}
WHERE #{backfill_via_table}.id = #{batch_table}.#{backfill_via_foreign_key}
AND #{batch_table}.id IN (#{sub_batch.select(:id).to_sql})
AND #{batch_table}.#{batch_column} IN (#{sub_batch.select(batch_column).to_sql})
SQL
end
end
......
......@@ -17,7 +17,7 @@ def construct_query(sub_batch:)
FROM #{backfill_via_table}
WHERE #{backfill_via_table}.id = #{batch_table}.#{backfill_via_foreign_key}
AND #{backfill_via_table}.#{partition_column} = #{batch_table}.#{partition_column}
AND #{batch_table}.id IN (#{sub_batch.select(:id).to_sql})
AND #{batch_table}.#{batch_column} IN (#{sub_batch.select(batch_column).to_sql})
SQL
end
end
......
......@@ -2,70 +2,120 @@
require 'spec_helper'
RSpec.describe Gitlab::BackgroundMigration::BackfillDesiredShardingKeyJob,
feature_category: :cell,
schema: 20231114034017 do
RSpec.describe Gitlab::BackgroundMigration::BackfillDesiredShardingKeyJob, migration: :gitlab_ci, feature_category: :cell do
let(:example_job_class) do
Class.new(described_class) do
operation_name :backfill_merge_request_diffs_project_id
operation_name :backfill_test_batch_table_project_id
feature_category :cell
end
end
let!(:start_id) { table(:merge_request_diffs).minimum(:id) }
let!(:end_id) { table(:merge_request_diffs).maximum(:id) }
let!(:migration) do
example_job_class.new(
let(:batch_table) { :_test_batch_table }
let(:batch_column) { :id }
let(:start_id) { table(batch_table).minimum(batch_column) }
let(:end_id) { table(batch_table).maximum(batch_column) }
let(:backfill_via_table) { :p_ci_builds }
let(:backfill_column) { :project_id }
let(:backfill_via_column) { :project_id }
let(:backfill_via_foreign_key) { :build_id }
let(:test_table) { table(batch_table) }
let(:connection) { ::Ci::ApplicationRecord.connection }
let(:migration_attrs) do
{
start_id: start_id,
end_id: end_id,
batch_table: :merge_request_diffs,
batch_column: :id,
batch_table: batch_table,
batch_column: batch_column,
sub_batch_size: 10,
pause_ms: 2,
connection: ::ApplicationRecord.connection,
connection: connection,
job_arguments: [
:project_id,
:merge_requests,
:target_project_id,
:merge_request_id
backfill_column,
backfill_via_table,
backfill_via_column,
backfill_via_foreign_key
]
)
}
end
let(:migration) { example_job_class.new(**migration_attrs) }
before do
connection.drop_table(batch_table, if_exists: true)
connection.create_table batch_table do |t|
t.timestamps_with_timezone null: false
t.integer :build_id, null: false
t.integer :project_id
end
end
after do
connection.drop_table(batch_table)
end
describe '#perform' do
let!(:diffs_without_project_id) do
13.times do
namespace = table(:namespaces).create!(name: 'my namespace', path: 'my-namespace')
project = table(:projects).create!(name: 'my project', path: 'my-project', namespace_id: namespace.id,
project_namespace_id: namespace.id)
merge_request = table(:merge_requests).create!(target_project_id: project.id, target_branch: 'main',
source_branch: 'not-main')
table(:merge_request_diffs).create!(merge_request_id: merge_request.id, project_id: nil)
let(:ci_pipelines_table) { table(:ci_pipelines, primary_key: :id) }
let(:ci_builds_table) { table(:p_ci_builds, primary_key: :id) }
let(:pipeline) { ci_pipelines_table.create!(partition_id: 100, project_id: 1) }
let!(:job1) { ci_builds_table.create!(partition_id: 100, project_id: 1, commit_id: pipeline.id) }
let!(:job2) { ci_builds_table.create!(partition_id: 100, project_id: 2, commit_id: pipeline.id) }
let(:test1) { test_table.create!(id: 1, build_id: job1.id, project_id: nil) }
let(:test2) { test_table.create!(id: 2, build_id: job2.id, project_id: nil) }
shared_examples 'a migration backfilling the missing project_id for the batch' do
it 'backfills the missing project_id for the batch' do
expect { migration.perform }
.to change { test1.reload.project_id }.from(nil).to(job1.project_id)
.and change { test2.reload.project_id }.from(nil).to(job2.project_id)
end
end
it 'backfills the missing project_id for the batch' do
backfilled_diffs = table(:merge_request_diffs)
.joins('INNER JOIN merge_requests ON merge_request_diffs.merge_request_id = merge_requests.id')
.where('merge_request_diffs.project_id = merge_requests.target_project_id')
context "when batch_column is id" do
let(:batch_column) { :id }
expect do
migration.perform
end.to change { backfilled_diffs.count }.from(0).to(13)
it_behaves_like 'a migration backfilling the missing project_id for the batch'
end
context "when batch_column is build_id" do
let(:batch_column) { :build_id }
it_behaves_like 'a migration backfilling the missing project_id for the batch'
end
context "when batch_column is invalid" do
let(:batch_column) { :project_id }
it 'does not backfill the missing project_id for the batch' do
expect { migration.perform }
.to not_change { test1.reload.project_id }.from(nil)
.and not_change { test2.reload.project_id }.from(nil)
end
end
end
describe '#constuct_query' do
it 'constructs a query using the supplied job arguments' do
sub_batch = table(:merge_request_diffs).all
expect(migration.construct_query(sub_batch: sub_batch)).to eq(<<~SQL)
UPDATE merge_request_diffs
SET project_id = merge_requests.target_project_id
FROM merge_requests
WHERE merge_requests.id = merge_request_diffs.merge_request_id
AND merge_request_diffs.id IN (#{sub_batch.select(:id).to_sql})
SQL
using RSpec::Parameterized::TableSyntax
where(:batch_column) do
[:id, :build_id]
end
with_them do
it 'constructs a query using the supplied job arguments' do
sub_batch = table(batch_table).all
expect(migration.construct_query(sub_batch: sub_batch)).to eq(<<~SQL)
UPDATE #{batch_table}
SET project_id = #{backfill_via_table}.#{backfill_via_column}
FROM #{backfill_via_table}
WHERE #{backfill_via_table}.id = #{batch_table}.#{backfill_via_foreign_key}
AND #{batch_table}.#{batch_column} IN (#{sub_batch.select(batch_column).to_sql})
SQL
end
end
end
end
......@@ -10,16 +10,17 @@
end
end
let(:start_id) { table(batch_table).minimum(:id) }
let(:end_id) { table(batch_table).maximum(:id) }
let(:batch_table) { :_test_batch_table }
let(:batch_column) { :id }
let(:backfill_via_table) { :p_ci_builds }
let(:start_id) { table(batch_table).minimum(batch_column) }
let(:end_id) { table(batch_table).maximum(batch_column) }
let(:batch_table) { :_test_batch_table }
let(:backfill_column) { :project_id }
let(:backfill_via_column) { :project_id }
let(:backfill_via_foreign_key) { :build_id }
let(:partition_column) { :partition_id }
let(:test_table) { table(:_test_batch_table) }
let(:test_table) { table(batch_table) }
let(:connection) { ::Ci::ApplicationRecord.connection }
let(:migration_attrs) do
......@@ -27,7 +28,7 @@
start_id: start_id,
end_id: end_id,
batch_table: batch_table,
batch_column: :id,
batch_column: batch_column,
sub_batch_size: 10,
pause_ms: 2,
connection: connection,
......@@ -44,7 +45,8 @@
let(:migration) { example_job_class.new(**migration_attrs) }
before do
connection.create_table :_test_batch_table do |t|
connection.drop_table(batch_table, if_exists: true)
connection.create_table batch_table do |t|
t.timestamps_with_timezone null: false
t.integer :build_id, null: false
t.integer :partition_id, null: false
......@@ -53,7 +55,7 @@
end
after do
connection.drop_table(:_test_batch_table)
connection.drop_table(batch_table)
end
describe '#perform' do
......@@ -67,25 +69,57 @@
let(:test1) { test_table.create!(id: 1, partition_id: 100, build_id: job1.id, project_id: nil) }
let(:test2) { test_table.create!(id: 2, partition_id: 100, build_id: job2.id, project_id: nil) }
it 'backfills the missing project_id for the batch' do
expect { migration.perform }
.to change { test1.reload.project_id }.from(nil).to(job1.project_id)
.and change { test2.reload.project_id }.from(nil).to(job2.project_id)
shared_examples 'a migration backfilling the missing project_id for the batch' do
it 'backfills the missing project_id for the batch' do
expect { migration.perform }
.to change { test1.reload.project_id }.from(nil).to(job1.project_id)
.and change { test2.reload.project_id }.from(nil).to(job2.project_id)
end
end
context "when batch_column is id" do
let(:batch_column) { :id }
it_behaves_like 'a migration backfilling the missing project_id for the batch'
end
context "when batch_column is build_id" do
let(:batch_column) { :build_id }
it_behaves_like 'a migration backfilling the missing project_id for the batch'
end
context "when batch_column is invalid" do
let(:batch_column) { :project_id }
it 'does not backfill the missing project_id for the batch' do
expect { migration.perform }
.to not_change { test1.reload.project_id }.from(nil)
.and not_change { test2.reload.project_id }.from(nil)
end
end
end
describe '#constuct_query' do
it 'constructs a query using the supplied job arguments' do
sub_batch = table(batch_table).all
expect(migration.construct_query(sub_batch: sub_batch)).to eq(<<~SQL)
UPDATE _test_batch_table
SET project_id = p_ci_builds.project_id
FROM p_ci_builds
WHERE p_ci_builds.id = _test_batch_table.build_id
AND p_ci_builds.partition_id = _test_batch_table.partition_id
AND _test_batch_table.id IN (#{sub_batch.select(:id).to_sql})
SQL
using RSpec::Parameterized::TableSyntax
where(:batch_column) do
[:id, :build_id]
end
with_them do
it 'constructs a query using the supplied job arguments' do
sub_batch = table(batch_table).all
expect(migration.construct_query(sub_batch: sub_batch)).to eq(<<~SQL)
UPDATE #{batch_table}
SET project_id = #{backfill_via_table}.#{backfill_via_column}
FROM #{backfill_via_table}
WHERE #{backfill_via_table}.id = #{batch_table}.#{backfill_via_foreign_key}
AND #{backfill_via_table}.#{partition_column} = #{batch_table}.#{partition_column}
AND #{batch_table}.#{batch_column} IN (#{sub_batch.select(batch_column).to_sql})
SQL
end
end
end
end
......@@ -35,24 +35,31 @@
}
end
let(:batch_column) { :id }
let!(:connection) { table(batch_table).connection }
let!(:starting_id) { table(batch_table).pluck(:id).min }
let!(:end_id) { table(batch_table).pluck(:id).max }
let!(:starting_id) { table(batch_table).pluck(batch_column).min }
let!(:end_id) { table(batch_table).pluck(batch_column).max }
let(:job_arguments) do
args = [
backfill_column,
backfill_via_table,
backfill_via_column,
backfill_via_foreign_key
]
args << partition_column if defined?(partition_column)
args
end
let!(:migration) do
described_class.new(
start_id: starting_id,
end_id: end_id,
batch_table: batch_table,
batch_column: :id,
batch_column: batch_column,
sub_batch_size: 10,
pause_ms: 2,
connection: connection,
job_arguments: [
backfill_column,
backfill_via_table,
backfill_via_column,
backfill_via_foreign_key
]
job_arguments: job_arguments
)
end
......@@ -63,6 +70,10 @@
it 'constructs a valid query' do
query = migration.construct_query(sub_batch: table(batch_table).all)
if defined?(partition_column)
expect(query).to include("AND #{backfill_via_table}.#{partition_column} = #{batch_table}.#{partition_column}")
end
if known_cross_joins.dig(batch_table, backfill_via_table).present?
::Gitlab::Database.allow_cross_joins_across_databases(
url: known_cross_joins[batch_table][backfill_via_table]
......
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment