Commit 38c2e480 authored by Micaël Bergeron's avatar Micaël Bergeron

shave off 30% of the query count

parent c6e26e24
......@@ -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
......
......@@ -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
......
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
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment