diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb index 935ffe343ff9f180c7d32b4bf3b6ea59fe340a65..3731b7c8577c66919ab6ca07b73119c36acfedc2 100644 --- a/app/models/concerns/issuable.rb +++ b/app/models/concerns/issuable.rb @@ -16,6 +16,7 @@ module Issuable include TimeTrackable include Importable include Editable + include AfterCommitQueue # This object is used to gather issuable meta data for displaying # upvotes, downvotes, notes and closing merge requests count for issues and merge requests diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb index 7df5039f2e405c313d8795456244112862a2be66..b84a6fd2b7d62bec29454b256f855a65de99901c 100644 --- a/app/services/issuable_base_service.rb +++ b/app/services/issuable_base_service.rb @@ -179,7 +179,6 @@ class IssuableBaseService < BaseService if params.present? && create_issuable(issuable, params, label_ids: label_ids) after_create(issuable) - issuable.create_cross_references!(current_user) execute_hooks(issuable) invalidate_cache_counts(issuable, users: issuable.assignees) end diff --git a/app/services/issues/create_service.rb b/app/services/issues/create_service.rb index 718a7ac1f2272145c5994501156bb74f26ee0619..9114f0ccc81bc4f0803b511286906fa40438be57 100644 --- a/app/services/issues/create_service.rb +++ b/app/services/issues/create_service.rb @@ -15,11 +15,14 @@ module Issues def before_create(issue) spam_check(issue, current_user) issue.move_to_end + + user = current_user + issue.run_after_commit do + NewIssueWorker.perform_async(issue.id, user.id) + end end def after_create(issuable) - event_service.open_issue(issuable, current_user) - notification_service.new_issue(issuable, current_user) todo_service.new_issue(issuable, current_user) user_agent_detail_service.create resolve_discussions_with_issue(issuable) diff --git a/app/services/merge_requests/create_service.rb b/app/services/merge_requests/create_service.rb index 5414fa79def34480d00f55c02bc7d079a31478b4..7d539fa49e69f6b5a785ea8f61bec725b7c2afb3 100644 --- a/app/services/merge_requests/create_service.rb +++ b/app/services/merge_requests/create_service.rb @@ -16,9 +16,15 @@ module MergeRequests create(merge_request) end + def before_create(merge_request) + user = current_user + merge_request.run_after_commit do + NewMergeRequestWorker.perform_async(merge_request.id, user.id) + end + end + def after_create(issuable) event_service.open_mr(issuable, current_user) - notification_service.new_merge_request(issuable, current_user) todo_service.new_merge_request(issuable, current_user) issuable.cache_merge_request_closes_issues!(current_user) update_merge_requests_head_pipeline(issuable) diff --git a/app/workers/concerns/new_issuable.rb b/app/workers/concerns/new_issuable.rb new file mode 100644 index 0000000000000000000000000000000000000000..3fd472bf0c1feb819da7dcd764de38a42a427807 --- /dev/null +++ b/app/workers/concerns/new_issuable.rb @@ -0,0 +1,23 @@ +module NewIssuable + attr_reader :issuable, :user + + def ensure_objects_found(issuable_id, user_id) + @issuable = issuable_class.find_by(id: issuable_id) + unless @issuable + log_error(issuable_class, issuable_id) + return false + end + + @user = User.find_by(id: user_id) + unless @user + log_error(User, user_id) + return false + end + + true + end + + def log_error(record_class, record_id) + Rails.logger.error("#{self.class}: couldn't find #{record_class} with ID=#{record_id}, skipping job") + end +end diff --git a/app/workers/new_issue_worker.rb b/app/workers/new_issue_worker.rb new file mode 100644 index 0000000000000000000000000000000000000000..19a778ad522c06f5de731856c51b67f9e2388797 --- /dev/null +++ b/app/workers/new_issue_worker.rb @@ -0,0 +1,17 @@ +class NewIssueWorker + include Sidekiq::Worker + include DedicatedSidekiqQueue + include NewIssuable + + def perform(issue_id, user_id) + return unless ensure_objects_found(issue_id, user_id) + + EventCreateService.new.open_issue(issuable, user) + NotificationService.new.new_issue(issuable, user) + issuable.create_cross_references!(user) + end + + def issuable_class + Issue + end +end diff --git a/app/workers/new_merge_request_worker.rb b/app/workers/new_merge_request_worker.rb new file mode 100644 index 0000000000000000000000000000000000000000..3c8a68016ff79dd39b72a954f46773d6b98803d1 --- /dev/null +++ b/app/workers/new_merge_request_worker.rb @@ -0,0 +1,17 @@ +class NewMergeRequestWorker + include Sidekiq::Worker + include DedicatedSidekiqQueue + include NewIssuable + + def perform(merge_request_id, user_id) + return unless ensure_objects_found(merge_request_id, user_id) + + EventCreateService.new.open_mr(issuable, user) + NotificationService.new.new_merge_request(issuable, user) + issuable.create_cross_references!(user) + end + + def issuable_class + MergeRequest + end +end diff --git a/changelogs/unreleased/32844-issuables-performance.yml b/changelogs/unreleased/32844-issuables-performance.yml new file mode 100644 index 0000000000000000000000000000000000000000..e9b21c1aa45cf5d6811da004343a27c1a6e3f879 --- /dev/null +++ b/changelogs/unreleased/32844-issuables-performance.yml @@ -0,0 +1,4 @@ +--- +title: Move some code from services to workers in order to improve performance +merge_request: 13326 +author: diff --git a/config/sidekiq_queues.yml b/config/sidekiq_queues.yml index 7496bfa4fbbfa72d17250e28e39a87d1e8b266b0..83abc83c9f0d76d656adc9960c0e6f8b0a5c3c72 100644 --- a/config/sidekiq_queues.yml +++ b/config/sidekiq_queues.yml @@ -23,6 +23,8 @@ - [update_merge_requests, 3] - [process_commit, 3] - [new_note, 2] + - [new_issue, 2] + - [new_merge_request, 2] - [build, 2] - [pipeline, 2] - [gitlab_shell, 2] diff --git a/spec/features/issues/create_branch_merge_request_spec.rb b/spec/features/issues/create_branch_merge_request_spec.rb index f59f687cf512c9fbd82cc5848a36713a705d4b10..546dc7e8a498c33c3c697a83e2edc054fabb9965 100644 --- a/spec/features/issues/create_branch_merge_request_spec.rb +++ b/spec/features/issues/create_branch_merge_request_spec.rb @@ -1,6 +1,6 @@ require 'rails_helper' -feature 'Create Branch/Merge Request Dropdown on issue page', js: true do +feature 'Create Branch/Merge Request Dropdown on issue page', :feature, :js do let(:user) { create(:user) } let!(:project) { create(:project, :repository) } let(:issue) { create(:issue, project: project, title: 'Cherry-Coloured Funk') } @@ -14,10 +14,14 @@ feature 'Create Branch/Merge Request Dropdown on issue page', js: true do it 'allows creating a merge request from the issue page' do visit project_issue_path(project, issue) - select_dropdown_option('create-mr') - - expect(page).to have_content('WIP: Resolve "Cherry-Coloured Funk"') - expect(current_path).to eq(project_merge_request_path(project, MergeRequest.first)) + perform_enqueued_jobs do + select_dropdown_option('create-mr') + + expect(page).to have_content('WIP: Resolve "Cherry-Coloured Funk"') + expect(current_path).to eq(project_merge_request_path(project, MergeRequest.first)) + + wait_for_requests + end visit project_issue_path(project, issue) diff --git a/spec/models/issue_spec.rb b/spec/models/issue_spec.rb index fa22eee3dea2e400fee8b02ac057c1bb9184289f..c055863d298051effee4665b9d2a6da1f1e6796d 100644 --- a/spec/models/issue_spec.rb +++ b/spec/models/issue_spec.rb @@ -191,14 +191,10 @@ describe Issue do end it 'returns the merge request to close this issue' do - mr - expect(issue.closed_by_merge_requests(mr.author)).to eq([mr]) end it "returns an empty array when the merge request is closed already" do - closed_mr - expect(issue.closed_by_merge_requests(closed_mr.author)).to eq([]) end diff --git a/spec/support/cycle_analytics_helpers.rb b/spec/support/cycle_analytics_helpers.rb index c0a5491a430917c2309cf5eef44658cb327b450b..30911e7fa86b28a5bf78fdfaa7a1718511c0f074 100644 --- a/spec/support/cycle_analytics_helpers.rb +++ b/spec/support/cycle_analytics_helpers.rb @@ -41,7 +41,9 @@ module CycleAnalyticsHelpers target_branch: 'master' } - MergeRequests::CreateService.new(project, user, opts).execute + mr = MergeRequests::CreateService.new(project, user, opts).execute + NewMergeRequestWorker.new.perform(mr, user) + mr end def merge_merge_requests_closing_issue(issue) diff --git a/spec/workers/new_issue_worker_spec.rb b/spec/workers/new_issue_worker_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..ed49ce57c0b303edf1e36e2a4cbb4db54973dca1 --- /dev/null +++ b/spec/workers/new_issue_worker_spec.rb @@ -0,0 +1,54 @@ +require 'spec_helper' + +describe NewIssueWorker do + describe '#perform' do + let(:worker) { described_class.new } + + context 'when an issue not found' do + it 'does not call Services' do + expect(EventCreateService).not_to receive(:new) + expect(NotificationService).not_to receive(:new) + + worker.perform(99, create(:user).id) + end + + it 'logs an error' do + expect(Rails.logger).to receive(:error).with('NewIssueWorker: couldn\'t find Issue with ID=99, skipping job') + + worker.perform(99, create(:user).id) + end + end + + context 'when a user not found' do + it 'does not call Services' do + expect(EventCreateService).not_to receive(:new) + expect(NotificationService).not_to receive(:new) + + worker.perform(create(:issue).id, 99) + end + + it 'logs an error' do + expect(Rails.logger).to receive(:error).with('NewIssueWorker: couldn\'t find User with ID=99, skipping job') + + worker.perform(create(:issue).id, 99) + end + end + + context 'when everything is ok' do + let(:project) { create(:project, :public) } + let(:mentioned) { create(:user) } + let(:user) { create(:user) } + let(:issue) { create(:issue, project: project, description: "issue for #{mentioned.to_reference}") } + + it 'creates a new event record' do + expect{ worker.perform(issue.id, user.id) }.to change { Event.count }.from(0).to(1) + end + + it 'creates a notification for the assignee' do + expect(Notify).to receive(:new_issue_email).with(mentioned.id, issue.id).and_return(double(deliver_later: true)) + + worker.perform(issue.id, user.id) + end + end + end +end diff --git a/spec/workers/new_merge_request_worker_spec.rb b/spec/workers/new_merge_request_worker_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..85af6184d39e72e7c5b127675b0bba00cc157280 --- /dev/null +++ b/spec/workers/new_merge_request_worker_spec.rb @@ -0,0 +1,56 @@ +require 'spec_helper' + +describe NewMergeRequestWorker do + describe '#perform' do + let(:worker) { described_class.new } + + context 'when a merge request not found' do + it 'does not call Services' do + expect(EventCreateService).not_to receive(:new) + expect(NotificationService).not_to receive(:new) + + worker.perform(99, create(:user).id) + end + + it 'logs an error' do + expect(Rails.logger).to receive(:error).with('NewMergeRequestWorker: couldn\'t find MergeRequest with ID=99, skipping job') + + worker.perform(99, create(:user).id) + end + end + + context 'when a user not found' do + it 'does not call Services' do + expect(EventCreateService).not_to receive(:new) + expect(NotificationService).not_to receive(:new) + + worker.perform(create(:merge_request).id, 99) + end + + it 'logs an error' do + expect(Rails.logger).to receive(:error).with('NewMergeRequestWorker: couldn\'t find User with ID=99, skipping job') + + worker.perform(create(:merge_request).id, 99) + end + end + + context 'when everything is ok' do + let(:project) { create(:project, :public) } + let(:mentioned) { create(:user) } + let(:user) { create(:user) } + let(:merge_request) do + create(:merge_request, source_project: project, description: "mr for #{mentioned.to_reference}") + end + + it 'creates a new event record' do + expect{ worker.perform(merge_request.id, user.id) }.to change { Event.count }.from(0).to(1) + end + + it 'creates a notification for the assignee' do + expect(Notify).to receive(:new_merge_request_email).with(mentioned.id, merge_request.id).and_return(double(deliver_later: true)) + + worker.perform(merge_request.id, user.id) + end + end + end +end