From 38c2e480bfa180241e94e77c049b1f5256d83bcf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mica=C3=ABl=20Bergeron?= Date: Wed, 6 Jun 2018 16:45:42 -0400 Subject: [PATCH 1/6] shave off 30% of the query count --- app/uploaders/file_uploader.rb | 6 +-- app/uploaders/object_storage.rb | 11 +++- .../migrate_uploads_worker_spec.rb | 50 +++++++++++++++---- 3 files changed, 53 insertions(+), 14 deletions(-) diff --git a/app/uploaders/file_uploader.rb b/app/uploaders/file_uploader.rb index 133fdf6684d..36bc0a4575a 100644 --- a/app/uploaders/file_uploader.rb +++ b/app/uploaders/file_uploader.rb @@ -65,10 +65,10 @@ class FileUploader < GitlabUploader SecureRandom.hex end - def upload_paths(filename) + def upload_paths(identifier) [ - File.join(secret, filename), - File.join(base_dir(Store::REMOTE), secret, filename) + File.join(secret, identifier), + File.join(base_dir(Store::REMOTE), secret, identifier) ] end diff --git a/app/uploaders/object_storage.rb b/app/uploaders/object_storage.rb index 5aa1bc7227c..3f5d0d200f4 100644 --- a/app/uploaders/object_storage.rb +++ b/app/uploaders/object_storage.rb @@ -29,7 +29,7 @@ module ObjectStorage end def retrieve_from_store!(identifier) - paths = store_dirs.map { |store, path| File.join(path, identifier) } + paths = upload_paths(identifier) unless current_upload_satisfies?(paths, model) # the upload we already have isn't right, find the correct one @@ -261,7 +261,7 @@ module ObjectStorage end def delete_migrated_file(migrated_file) - migrated_file.delete if exists? + migrated_file.delete end def exists? @@ -279,6 +279,13 @@ module ObjectStorage } end + # Returns all the possible paths for an upload. + # the `upload.path` is a lookup parameter, and it may change + # depending on the `store` param. + def upload_paths(identifier) + store_dirs.map { |store, path| File.join(path, identifier) } + end + def cache!(new_file = sanitized_file) # We intercept ::UploadedFile which might be stored on remote storage # We use that for "accelerated" uploads, where we store result on remote storage diff --git a/spec/uploaders/workers/object_storage/migrate_uploads_worker_spec.rb b/spec/uploaders/workers/object_storage/migrate_uploads_worker_spec.rb index aed62f97448..18da0c6d39a 100644 --- a/spec/uploaders/workers/object_storage/migrate_uploads_worker_spec.rb +++ b/spec/uploaders/workers/object_storage/migrate_uploads_worker_spec.rb @@ -1,5 +1,7 @@ require 'spec_helper' +MIGRATION_QUERIES = 7 + describe ObjectStorage::MigrateUploadsWorker, :sidekiq do shared_context 'sanity_check! fails' do before do @@ -11,6 +13,13 @@ describe ObjectStorage::MigrateUploadsWorker, :sidekiq do let(:uploads) { Upload.all } let(:to_store) { ObjectStorage::Store::REMOTE } + def perform(uploads) + binding.pry + described_class.new.perform(uploads.ids, model_class.to_s, mounted_as, to_store) + rescue ObjectStorage::MigrateUploadsWorker::Report::MigrationFailures + # swallow + end + shared_examples "uploads migration worker" do describe '.enqueue!' do def enqueue! @@ -69,12 +78,6 @@ describe ObjectStorage::MigrateUploadsWorker, :sidekiq do end describe '#perform' do - def perform - described_class.new.perform(uploads.ids, model_class.to_s, mounted_as, to_store) - rescue ObjectStorage::MigrateUploadsWorker::Report::MigrationFailures - # swallow - end - shared_examples 'outputs correctly' do |success: 0, failures: 0| total = success + failures @@ -82,7 +85,7 @@ describe ObjectStorage::MigrateUploadsWorker, :sidekiq do it 'outputs the reports' do expect(Rails.logger).to receive(:info).with(%r{Migrated #{success}/#{total} files}) - perform + perform(uploads) end end @@ -90,7 +93,7 @@ describe ObjectStorage::MigrateUploadsWorker, :sidekiq do it 'outputs upload failures' do expect(Rails.logger).to receive(:warn).with(/Error .* I am a teapot/) - perform + perform(uploads) end end end @@ -98,7 +101,7 @@ describe ObjectStorage::MigrateUploadsWorker, :sidekiq do it_behaves_like 'outputs correctly', success: 10 it 'migrates files' do - perform + perform(uploads) expect(Upload.where(store: ObjectStorage::Store::LOCAL).count).to eq(0) end @@ -123,6 +126,18 @@ describe ObjectStorage::MigrateUploadsWorker, :sidekiq do end it_behaves_like "uploads migration worker" + + describe "limits N+1 queries" do + let!(:projects) { create_list(:project, 10, :with_avatar) } + + it do + query_count = ActiveRecord::QueryRecorder.new { perform(uploads) } + + more_projects = create_list(:project, 100, :with_avatar) + expected_queries_per_migration = MIGRATION_QUERIES * more_projects.count + expect { perform(Upload.all) }.not_to exceed_query_limit(query_count).with_threshold(expected_queries_per_migration) + end + end end context "for FileUploader" do @@ -140,5 +155,22 @@ describe ObjectStorage::MigrateUploadsWorker, :sidekiq do end it_behaves_like "uploads migration worker" + + describe "limits N+1 queries" do + let!(:projects) { create_list(:project, 10) } + + it do + query_count = ActiveRecord::QueryRecorder.new { perform(uploads) } + + more_projects = create_list(:project, 100) + more_projects.map do |project| + uploader = FileUploader.new(project) + uploader.store!(fixture_file_upload('spec/fixtures/doc_sample.txt')) + end + expected_queries_per_migration = MIGRATION_QUERIES * more_projects.count + + expect { perform(Upload.all) }.not_to exceed_query_limit(query_count).with_threshold(expected_queries_per_migration) + end + end end end From 44975f8a5ad9c40c615f47f683fb46c94aa0e130 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mica=C3=ABl=20Bergeron?= Date: Thu, 7 Jun 2018 10:01:47 -0400 Subject: [PATCH 2/6] shave off another 20% query --- app/uploaders/object_storage.rb | 7 ++++--- app/uploaders/records_uploads.rb | 2 +- .../workers/object_storage/migrate_uploads_worker_spec.rb | 3 +-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/app/uploaders/object_storage.rb b/app/uploaders/object_storage.rb index 3f5d0d200f4..bc8f1a5861a 100644 --- a/app/uploaders/object_storage.rb +++ b/app/uploaders/object_storage.rb @@ -376,12 +376,13 @@ module ObjectStorage end def with_exclusive_lease - uuid = Gitlab::ExclusiveLease.new(exclusive_lease_key, timeout: 1.hour.to_i).try_obtain - raise 'exclusive lease already taken' unless uuid + lease_key = exclusive_lease_key + uuid = Gitlab::ExclusiveLease.new(lease_key, timeout: 1.hour.to_i).try_obtain + raise "Exclusive lease #{lease_key} already taken." unless uuid yield uuid ensure - Gitlab::ExclusiveLease.cancel(exclusive_lease_key, uuid) + Gitlab::ExclusiveLease.cancel(lease_key, uuid) end # diff --git a/app/uploaders/records_uploads.rb b/app/uploaders/records_uploads.rb index 89c74a78835..301f4681fcd 100644 --- a/app/uploaders/records_uploads.rb +++ b/app/uploaders/records_uploads.rb @@ -22,7 +22,7 @@ module RecordsUploads Upload.transaction do uploads.where(path: upload_path).delete_all - upload.destroy! if upload + upload.delete if upload self.upload = build_upload.tap(&:save!) end diff --git a/spec/uploaders/workers/object_storage/migrate_uploads_worker_spec.rb b/spec/uploaders/workers/object_storage/migrate_uploads_worker_spec.rb index 18da0c6d39a..ba01cfe53c5 100644 --- a/spec/uploaders/workers/object_storage/migrate_uploads_worker_spec.rb +++ b/spec/uploaders/workers/object_storage/migrate_uploads_worker_spec.rb @@ -1,6 +1,6 @@ require 'spec_helper' -MIGRATION_QUERIES = 7 +MIGRATION_QUERIES = 5 describe ObjectStorage::MigrateUploadsWorker, :sidekiq do shared_context 'sanity_check! fails' do @@ -14,7 +14,6 @@ describe ObjectStorage::MigrateUploadsWorker, :sidekiq do let(:to_store) { ObjectStorage::Store::REMOTE } def perform(uploads) - binding.pry described_class.new.perform(uploads.ids, model_class.to_s, mounted_as, to_store) rescue ObjectStorage::MigrateUploadsWorker::Report::MigrationFailures # swallow From a667de18d3b1d798992e1441ce774c63f801c07e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mica=C3=ABl=20Bergeron?= Date: Thu, 7 Jun 2018 10:15:22 -0400 Subject: [PATCH 3/6] added changelog --- ...igrateuploadsworker-is-doing-n-1-queries-on-migration.yml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelogs/unreleased/47408-migrateuploadsworker-is-doing-n-1-queries-on-migration.yml diff --git a/changelogs/unreleased/47408-migrateuploadsworker-is-doing-n-1-queries-on-migration.yml b/changelogs/unreleased/47408-migrateuploadsworker-is-doing-n-1-queries-on-migration.yml new file mode 100644 index 00000000000..c0df82f35f1 --- /dev/null +++ b/changelogs/unreleased/47408-migrateuploadsworker-is-doing-n-1-queries-on-migration.yml @@ -0,0 +1,5 @@ +--- +title: Optimize the upload migration proces +merge_request: 15947 +author: +type: fixed From 50872bcc242a582c7e3af25df4d32e1c3e0a28f3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mica=C3=ABl=20Bergeron?= Date: Thu, 7 Jun 2018 11:06:04 -0400 Subject: [PATCH 4/6] fix the failing spec --- app/uploaders/object_storage.rb | 3 ++- .../uploaders/object_storage_shared_examples.rb | 4 ++-- spec/uploaders/object_storage_spec.rb | 4 ++-- 3 files changed, 6 insertions(+), 5 deletions(-) diff --git a/app/uploaders/object_storage.rb b/app/uploaders/object_storage.rb index bc8f1a5861a..bebaa3b807b 100644 --- a/app/uploaders/object_storage.rb +++ b/app/uploaders/object_storage.rb @@ -9,6 +9,7 @@ module ObjectStorage RemoteStoreError = Class.new(StandardError) UnknownStoreError = Class.new(StandardError) ObjectStorageUnavailable = Class.new(StandardError) + ExclusiveLeaseTaken = Class.new(StandardError) TMP_UPLOAD_PATH = 'tmp/uploads'.freeze @@ -378,7 +379,7 @@ module ObjectStorage def with_exclusive_lease lease_key = exclusive_lease_key uuid = Gitlab::ExclusiveLease.new(lease_key, timeout: 1.hour.to_i).try_obtain - raise "Exclusive lease #{lease_key} already taken." unless uuid + raise ExclusiveLeaseTaken, "Exclusive lease #{lease_key} already taken." unless uuid yield uuid ensure diff --git a/spec/support/shared_examples/uploaders/object_storage_shared_examples.rb b/spec/support/shared_examples/uploaders/object_storage_shared_examples.rb index 6352f1527cd..1ecddc14d58 100644 --- a/spec/support/shared_examples/uploaders/object_storage_shared_examples.rb +++ b/spec/support/shared_examples/uploaders/object_storage_shared_examples.rb @@ -85,13 +85,13 @@ shared_examples "migrates" do |to_store:, from_store: nil| it 'does not execute migrate!' do expect(subject).not_to receive(:unsafe_migrate!) - expect { migrate(to) }.to raise_error('exclusive lease already taken') + expect { migrate(to) }.to raise_error(ObjectStorage::ExclusiveLeaseTaken) end it 'does not execute use_file' do expect(subject).not_to receive(:unsafe_use_file) - expect { subject.use_file }.to raise_error('exclusive lease already taken') + expect { subject.use_file }.to raise_error(ObjectStorage::ExclusiveLeaseTaken) end after do diff --git a/spec/uploaders/object_storage_spec.rb b/spec/uploaders/object_storage_spec.rb index 4503288e410..03386bf764f 100644 --- a/spec/uploaders/object_storage_spec.rb +++ b/spec/uploaders/object_storage_spec.rb @@ -321,7 +321,7 @@ describe ObjectStorage do when_file_is_in_use do expect(uploader).not_to receive(:unsafe_migrate!) - expect { uploader.migrate!(described_class::Store::REMOTE) }.to raise_error('exclusive lease already taken') + expect { uploader.migrate!(described_class::Store::REMOTE) }.to raise_error(ObjectStorage::ExclusiveLeaseTaken) end end @@ -329,7 +329,7 @@ describe ObjectStorage do when_file_is_in_use do expect(uploader).not_to receive(:unsafe_use_file) - expect { uploader.use_file }.to raise_error('exclusive lease already taken') + expect { uploader.use_file }.to raise_error(ObjectStorage::ExclusiveLeaseTaken) end end end From e1589a5c584acae83d97d41494616be1f3981da7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mica=C3=ABl=20Bergeron?= Date: Fri, 8 Jun 2018 10:51:59 -0400 Subject: [PATCH 5/6] apply feedback --- app/uploaders/object_storage.rb | 13 +++++++++++-- .../object_storage/migrate_uploads_worker_spec.rb | 4 ++-- 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/app/uploaders/object_storage.rb b/app/uploaders/object_storage.rb index bebaa3b807b..43c2b419332 100644 --- a/app/uploaders/object_storage.rb +++ b/app/uploaders/object_storage.rb @@ -9,7 +9,16 @@ module ObjectStorage RemoteStoreError = Class.new(StandardError) UnknownStoreError = Class.new(StandardError) ObjectStorageUnavailable = Class.new(StandardError) - ExclusiveLeaseTaken = Class.new(StandardError) + + class ExclusiveLeaseTaken < StandardError + def initialize(lease_key) + @lease_key = lease_key + end + + def message + "Exclusive lease #{@lease_key} already taken." + end + end TMP_UPLOAD_PATH = 'tmp/uploads'.freeze @@ -379,7 +388,7 @@ module ObjectStorage def with_exclusive_lease lease_key = exclusive_lease_key uuid = Gitlab::ExclusiveLease.new(lease_key, timeout: 1.hour.to_i).try_obtain - raise ExclusiveLeaseTaken, "Exclusive lease #{lease_key} already taken." unless uuid + raise ExclusiveLeaseTaken.new(lease_key) unless uuid yield uuid ensure diff --git a/spec/uploaders/workers/object_storage/migrate_uploads_worker_spec.rb b/spec/uploaders/workers/object_storage/migrate_uploads_worker_spec.rb index ba01cfe53c5..31d323626c5 100644 --- a/spec/uploaders/workers/object_storage/migrate_uploads_worker_spec.rb +++ b/spec/uploaders/workers/object_storage/migrate_uploads_worker_spec.rb @@ -129,7 +129,7 @@ describe ObjectStorage::MigrateUploadsWorker, :sidekiq do describe "limits N+1 queries" do let!(:projects) { create_list(:project, 10, :with_avatar) } - it do + it "to N*#{MIGRATION_QUERIES}" do query_count = ActiveRecord::QueryRecorder.new { perform(uploads) } more_projects = create_list(:project, 100, :with_avatar) @@ -158,7 +158,7 @@ describe ObjectStorage::MigrateUploadsWorker, :sidekiq do describe "limits N+1 queries" do let!(:projects) { create_list(:project, 10) } - it do + it "to N*#{MIGRATION_QUERIES}" do query_count = ActiveRecord::QueryRecorder.new { perform(uploads) } more_projects = create_list(:project, 100) From 3d42bab71ad293c99d029dfb4f0c9aa0378643d4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mica=C3=ABl=20Bergeron?= Date: Tue, 12 Jun 2018 11:22:35 -0400 Subject: [PATCH 6/6] apply feedback --- app/uploaders/object_storage.rb | 3 +- .../migrate_uploads_worker_spec.rb | 34 ++++++++----------- 2 files changed, 16 insertions(+), 21 deletions(-) diff --git a/app/uploaders/object_storage.rb b/app/uploaders/object_storage.rb index 43c2b419332..14983943f76 100644 --- a/app/uploaders/object_storage.rb +++ b/app/uploaders/object_storage.rb @@ -16,7 +16,8 @@ module ObjectStorage end def message - "Exclusive lease #{@lease_key} already taken." + *lease_key_group, _ = *@lease_key.split(":") + "Exclusive lease for #{lease_key_group.join(':')} is already taken." end end diff --git a/spec/uploaders/workers/object_storage/migrate_uploads_worker_spec.rb b/spec/uploaders/workers/object_storage/migrate_uploads_worker_spec.rb index 31d323626c5..da490cb02af 100644 --- a/spec/uploaders/workers/object_storage/migrate_uploads_worker_spec.rb +++ b/spec/uploaders/workers/object_storage/migrate_uploads_worker_spec.rb @@ -1,7 +1,5 @@ require 'spec_helper' -MIGRATION_QUERIES = 5 - describe ObjectStorage::MigrateUploadsWorker, :sidekiq do shared_context 'sanity_check! fails' do before do @@ -127,13 +125,12 @@ describe ObjectStorage::MigrateUploadsWorker, :sidekiq do it_behaves_like "uploads migration worker" describe "limits N+1 queries" do - let!(:projects) { create_list(:project, 10, :with_avatar) } - - it "to N*#{MIGRATION_QUERIES}" do + it "to N*5" do query_count = ActiveRecord::QueryRecorder.new { perform(uploads) } - more_projects = create_list(:project, 100, :with_avatar) - expected_queries_per_migration = MIGRATION_QUERIES * more_projects.count + more_projects = create_list(:project, 3, :with_avatar) + + expected_queries_per_migration = 5 * more_projects.count expect { perform(Upload.all) }.not_to exceed_query_limit(query_count).with_threshold(expected_queries_per_migration) end end @@ -144,30 +141,27 @@ describe ObjectStorage::MigrateUploadsWorker, :sidekiq do let(:secret) { SecureRandom.hex } let(:mounted_as) { nil } + def upload_file(project) + uploader = FileUploader.new(project) + uploader.store!(fixture_file_upload('spec/fixtures/doc_sample.txt')) + end + before do stub_uploads_object_storage(FileUploader) - projects.map do |project| - uploader = FileUploader.new(project) - uploader.store!(fixture_file_upload('spec/fixtures/doc_sample.txt')) - end + projects.map(&method(:upload_file)) end it_behaves_like "uploads migration worker" describe "limits N+1 queries" do - let!(:projects) { create_list(:project, 10) } - - it "to N*#{MIGRATION_QUERIES}" do + it "to N*5" do query_count = ActiveRecord::QueryRecorder.new { perform(uploads) } - more_projects = create_list(:project, 100) - more_projects.map do |project| - uploader = FileUploader.new(project) - uploader.store!(fixture_file_upload('spec/fixtures/doc_sample.txt')) - end - expected_queries_per_migration = MIGRATION_QUERIES * more_projects.count + more_projects = create_list(:project, 3) + more_projects.map(&method(:upload_file)) + expected_queries_per_migration = 5 * more_projects.count expect { perform(Upload.all) }.not_to exceed_query_limit(query_count).with_threshold(expected_queries_per_migration) end end