summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorJarka Kadlecova <jarka@gitlab.com>2017-08-01 14:38:45 +0200
committerJarka Kadlecova <jarka@gitlab.com>2017-08-07 15:57:56 +0200
commit9ef3c431e4859e1bc03267735b956d5920d5dd42 (patch)
tree5a4063641e79766c249924f5095e6c328a279846
parentfaa2a123911eaf84bb57163ea7af759d4632601b (diff)
downloadgitlab-ce-9ef3c431e4859e1bc03267735b956d5920d5dd42.tar.gz
Move some after_create parts to worker to improve performance32844-issuables-performance
-rw-r--r--app/models/concerns/issuable.rb1
-rw-r--r--app/services/issuable_base_service.rb1
-rw-r--r--app/services/issues/create_service.rb7
-rw-r--r--app/services/merge_requests/create_service.rb8
-rw-r--r--app/workers/concerns/new_issuable.rb23
-rw-r--r--app/workers/new_issue_worker.rb17
-rw-r--r--app/workers/new_merge_request_worker.rb17
-rw-r--r--changelogs/unreleased/32844-issuables-performance.yml4
-rw-r--r--config/sidekiq_queues.yml2
-rw-r--r--spec/features/issues/create_branch_merge_request_spec.rb14
-rw-r--r--spec/models/issue_spec.rb4
-rw-r--r--spec/support/cycle_analytics_helpers.rb4
-rw-r--r--spec/workers/new_issue_worker_spec.rb54
-rw-r--r--spec/workers/new_merge_request_worker_spec.rb56
14 files changed, 198 insertions, 14 deletions
diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb
index 935ffe343ff..3731b7c8577 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 ea497729115..0e25c555136 100644
--- a/app/services/issuable_base_service.rb
+++ b/app/services/issuable_base_service.rb
@@ -182,7 +182,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 718a7ac1f22..9114f0ccc81 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 19189e64acf..eb04048b748 100644
--- a/app/services/merge_requests/create_service.rb
+++ b/app/services/merge_requests/create_service.rb
@@ -17,9 +17,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)
end
diff --git a/app/workers/concerns/new_issuable.rb b/app/workers/concerns/new_issuable.rb
new file mode 100644
index 00000000000..3fd472bf0c1
--- /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 00000000000..19a778ad522
--- /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 00000000000..3c8a68016ff
--- /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 00000000000..e9b21c1aa45
--- /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 7496bfa4fbb..83abc83c9f0 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 f59f687cf51..546dc7e8a49 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 d72790eefe5..c7b41ab6f4b 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 c0a5491a430..30911e7fa86 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 00000000000..ed49ce57c0b
--- /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 00000000000..85af6184d39
--- /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