Perform pull request IO work outside a transactiongh-importer-transactions

When importing a GitHub pull request we would perform all work in a
single database transaction. This is less than ideal, because we perform
various slow Git operations when creating a merge request. This in turn
can lead to many DB connections being used, while just waiting for an IO
operation to complete.

To work around this, we now move most of the heavy lifting out of the
database transaction. Some extra error handling is added to ensure we
can resume importing a partially imported pull request, instead of just
throwing an error.

This commit also changes the specs for IssueImporter so they don't rely
on deprecated RSpec methods.

2 files changed, 74 insertions, 15 deletions

diff --git a/spec/lib/gitlab/github_import/importer/issue_importer_spec.rb b/spec/lib/gitlab/github_import/importer/issue_importer_spec.rb
index d34ca0b76b8..81fe97c1e49 100644
--- a/spec/lib/gitlab/github_import/importer/issue_importer_spec.rb
+++ b/spec/lib/gitlab/github_import/importer/issue_importer_spec.rb
@@ -180,12 +180,12 @@ describe Gitlab::GithubImport::Importer::IssueImporter, :clean_gitlab_redis_cach
 
       allow(importer.user_finder)
         .to receive(:user_id_for)
-        .ordered.with(issue.assignees[0])
+        .with(issue.assignees[0])
         .and_return(4)
 
       allow(importer.user_finder)
         .to receive(:user_id_for)
-        .ordered.with(issue.assignees[1])
+        .with(issue.assignees[1])
         .and_return(5)
 
       expect(Gitlab::Database)
diff --git a/spec/lib/gitlab/github_import/importer/pull_request_importer_spec.rb b/spec/lib/gitlab/github_import/importer/pull_request_importer_spec.rb
index 35f3fdf8304..6686b7ce0b5 100644
--- a/spec/lib/gitlab/github_import/importer/pull_request_importer_spec.rb
+++ b/spec/lib/gitlab/github_import/importer/pull_request_importer_spec.rb
@@ -40,13 +40,19 @@ describe Gitlab::GithubImport::Importer::PullRequestImporter, :clean_gitlab_redi
 
   describe '#execute' do
     it 'imports the pull request' do
+      mr = double(:merge_request, id: 10)
+
       expect(importer)
         .to receive(:create_merge_request)
-        .and_return(10)
+        .and_return([mr, false])
+
+      expect(importer)
+        .to receive(:insert_git_data)
+        .with(mr, false)
 
       expect_any_instance_of(Gitlab::GithubImport::IssuableFinder)
         .to receive(:cache_database_id)
-        .with(10)
+        .with(mr.id)
 
       importer.execute
     end
@@ -99,18 +105,11 @@ describe Gitlab::GithubImport::Importer::PullRequestImporter, :clean_gitlab_redi
         importer.create_merge_request
       end
 
-      it 'returns the ID of the created merge request' do
-        id = importer.create_merge_request
-
-        expect(id).to be_a_kind_of(Numeric)
-      end
-
-      it 'creates the merge request diffs' do
-        importer.create_merge_request
-
-        mr = project.merge_requests.take
+      it 'returns the created merge request' do
+        mr, exists = importer.create_merge_request
 
-        expect(mr.merge_request_diffs.exists?).to eq(true)
+        expect(mr).to be_instance_of(MergeRequest)
+        expect(exists).to eq(false)
       end
     end
 
@@ -217,5 +216,65 @@ describe Gitlab::GithubImport::Importer::PullRequestImporter, :clean_gitlab_redi
         expect { importer.create_merge_request }.not_to raise_error
       end
     end
+
+    context 'when the merge request already exists' do
+      before do
+        allow(importer.user_finder)
+          .to receive(:author_id_for)
+          .with(pull_request)
+          .and_return([user.id, true])
+
+        allow(importer.user_finder)
+          .to receive(:assignee_id_for)
+          .with(pull_request)
+          .and_return(user.id)
+      end
+
+      it 'returns the existing merge request' do
+        mr1, exists1 = importer.create_merge_request
+        mr2, exists2 = importer.create_merge_request
+
+        expect(mr2).to eq(mr1)
+        expect(exists1).to eq(false)
+        expect(exists2).to eq(true)
+      end
+    end
+  end
+
+  describe '#insert_git_data' do
+    before do
+      allow(importer.milestone_finder)
+        .to receive(:id_for)
+        .with(pull_request)
+        .and_return(milestone.id)
+
+      allow(importer.user_finder)
+        .to receive(:author_id_for)
+        .with(pull_request)
+        .and_return([user.id, true])
+
+      allow(importer.user_finder)
+        .to receive(:assignee_id_for)
+        .with(pull_request)
+        .and_return(user.id)
+    end
+
+    it 'creates the merge request diffs' do
+      mr, exists = importer.create_merge_request
+
+      importer.insert_git_data(mr, exists)
+
+      expect(mr.merge_request_diffs.exists?).to eq(true)
+    end
+
+    it 'creates the merge request diff commits' do
+      mr, exists = importer.create_merge_request
+
+      importer.insert_git_data(mr, exists)
+
+      diff = mr.merge_request_diffs.take
+
+      expect(diff.merge_request_diff_commits.exists?).to eq(true)
+    end
   end
 end