From 09cdd7dca0916f9edec3e531c6787a6050bc295f Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Fri, 24 Aug 2018 12:58:50 -0700 Subject: Bitbucket Server importer: Eliminate most idle-in-transaction issues Just like with the GitHub importer, the Bitbucket Server importer can hit the default 60 s idle-in-transaction timeouts if it takes too long to create the merge request. We solve this by using the same approach as the GitHub importer: 1. Bypass all validation and hooks in creating a merge request 2. Insert the Git data in a separate transaction Part of #50021 --- .../sh-insert-git-data-in-separate-transaction.yml | 5 +++ lib/gitlab/bitbucket_server_import/importer.rb | 18 +++++---- lib/gitlab/import/merge_request_creator.rb | 40 ++++++++++++++++++++ .../gitlab/import/merge_request_creator_spec.rb | 43 ++++++++++++++++++++++ 4 files changed, 98 insertions(+), 8 deletions(-) create mode 100644 changelogs/unreleased/sh-insert-git-data-in-separate-transaction.yml create mode 100644 lib/gitlab/import/merge_request_creator.rb create mode 100644 spec/lib/gitlab/import/merge_request_creator_spec.rb diff --git a/changelogs/unreleased/sh-insert-git-data-in-separate-transaction.yml b/changelogs/unreleased/sh-insert-git-data-in-separate-transaction.yml new file mode 100644 index 00000000000..116929b2f53 --- /dev/null +++ b/changelogs/unreleased/sh-insert-git-data-in-separate-transaction.yml @@ -0,0 +1,5 @@ +--- +title: 'Bitbucket Server importer: Eliminate most idle-in-transaction issues' +merge_request: +author: +type: performance diff --git a/lib/gitlab/bitbucket_server_import/importer.rb b/lib/gitlab/bitbucket_server_import/importer.rb index 268d21a77d1..b591d94668f 100644 --- a/lib/gitlab/bitbucket_server_import/importer.rb +++ b/lib/gitlab/bitbucket_server_import/importer.rb @@ -1,7 +1,10 @@ +# frozen_string_literal: true + module Gitlab module BitbucketServerImport class Importer include Gitlab::ShellAdapter + attr_reader :recover_missing_commits attr_reader :project, :project_key, :repository_slug, :client, :errors, :users @@ -175,21 +178,18 @@ module Gitlab description = '' description += @formatter.author_line(pull_request.author) unless find_user_id(pull_request.author_email) description += pull_request.description if pull_request.description - - source_branch_sha = pull_request.source_branch_sha - target_branch_sha = pull_request.target_branch_sha author_id = gitlab_user_id(pull_request.author_email) attributes = { iid: pull_request.iid, title: pull_request.title, description: description, - source_project: project, + source_project_id: project.id, source_branch: Gitlab::Git.ref_name(pull_request.source_branch_name), - source_branch_sha: source_branch_sha, - target_project: project, + source_branch_sha: pull_request.source_branch_sha, + target_project_id: project.id, target_branch: Gitlab::Git.ref_name(pull_request.target_branch_name), - target_branch_sha: target_branch_sha, + target_branch_sha: pull_request.target_branch_sha, state: pull_request.state, author_id: author_id, assignee_id: nil, @@ -197,7 +197,9 @@ module Gitlab updated_at: pull_request.updated_at } - merge_request = project.merge_requests.create!(attributes) + creator = Gitlab::Import::MergeRequestCreator.new(project) + merge_request = creator.execute(attributes) + import_pull_request_comments(pull_request, merge_request) if merge_request.persisted? end diff --git a/lib/gitlab/import/merge_request_creator.rb b/lib/gitlab/import/merge_request_creator.rb new file mode 100644 index 00000000000..a01951b0762 --- /dev/null +++ b/lib/gitlab/import/merge_request_creator.rb @@ -0,0 +1,40 @@ +# frozen_string_literal: true + +# This module is designed for importers that need to create many merge +# requests quickly. When creating merge requests there are a lot of hooks +# that may run, for many different reasons. Many of these hooks (e.g. the ones +# used for rendering Markdown) are completely unnecessary and may even lead to +# transaction timeouts. +# +# To ensure importing merge requests requests has a minimal impact and can +# complete in a reasonable time we bypass all the hooks by inserting the row +# and then retrieving it. We then only perform the additional work that is +# strictly necessary. +module Gitlab + module Import + class MergeRequestCreator + include ::Gitlab::Import::DatabaseHelpers + include ::Gitlab::Import::MergeRequestHelpers + + attr_accessor :project + + def initialize(project) + @project = project + end + + def execute(attributes) + source_branch_sha = attributes.delete(:source_branch_sha) + target_branch_sha = attributes.delete(:target_branch_sha) + iid = attributes[:iid] + + merge_request, already_exists = create_merge_request_without_hooks(project, attributes, iid) + + if merge_request + insert_or_replace_git_data(merge_request, source_branch_sha, target_branch_sha, already_exists) + end + + merge_request + end + end + end +end diff --git a/spec/lib/gitlab/import/merge_request_creator_spec.rb b/spec/lib/gitlab/import/merge_request_creator_spec.rb new file mode 100644 index 00000000000..cd3359da341 --- /dev/null +++ b/spec/lib/gitlab/import/merge_request_creator_spec.rb @@ -0,0 +1,43 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Gitlab::Import::MergeRequestCreator do + let(:project) { create(:project, :repository) } + + subject { described_class.new(project) } + + describe '#execute' do + context 'merge request already exists' do + let(:merge_request) { create(:merge_request, target_project: project, source_project: project) } + let(:commits) { merge_request.merge_request_diffs.first.commits } + let(:attributes) { HashWithIndifferentAccess.new(merge_request.attributes) } + + it 'updates the data' do + commits_count = commits.count + merge_request.merge_request_diffs.destroy_all # rubocop: disable DestroyAll + + expect(merge_request.merge_request_diffs.count).to eq(0) + + subject.execute(attributes) + + expect(merge_request.reload.merge_request_diffs.count).to eq(1) + expect(merge_request.reload.merge_request_diffs.first.commits.count).to eq(commits_count) + end + end + + context 'new merge request' do + let(:merge_request) { build(:merge_request, target_project: project, source_project: project) } + let(:attributes) { HashWithIndifferentAccess.new(merge_request.attributes) } + + it 'creates a new merge request' do + attributes.delete(:id) + + expect { subject.execute(attributes) }.to change { MergeRequest.count }.by(1) + + new_mr = MergeRequest.last + expect(new_mr.merge_request_diffs.count).to eq(1) + end + end + end +end -- cgit v1.2.1