Merge branch 'ajk-issue-placement-worker-deduplication' into 'master'

Pass project ID to issue placement worker

See merge request gitlab-org/gitlab!42091

app/services/issues/create_service.rb

@@ -21,7 +21,7 @@ module Issues
       user = current_user
       issue.run_after_commit do
         NewIssueWorker.perform_async(issue.id, user.id)
-        IssuePlacementWorker.perform_async(issue.id)
+        IssuePlacementWorker.perform_async(nil, issue.project_id)
       end
     end
 

app/workers/issue_placement_worker.rb

@@ -14,7 +14,7 @@ class IssuePlacementWorker
 
   # rubocop: disable CodeReuse/ActiveRecord
   def perform(issue_id, project_id = nil)
-    issue = Issue.id_in(issue_id).first
+    issue = find_issue(issue_id, project_id)
     return unless issue
 
     # Move the oldest 100 unpositioned items to the end.
@@ -31,10 +31,19 @@ class IssuePlacementWorker
 
     Issue.move_nulls_to_end(to_place)
     Issues::BaseService.new(nil).rebalance_if_needed(to_place.max_by(&:relative_position))
-    IssuePlacementWorker.perform_async(leftover.id) if leftover.present?
+    IssuePlacementWorker.perform_async(nil, leftover.project_id) if leftover.present?
   rescue RelativePositioning::NoSpaceLeft => e
-    Gitlab::ErrorTracking.log_exception(e, issue_id: issue_id)
-    IssueRebalancingWorker.perform_async(nil, issue.project_id)
+    Gitlab::ErrorTracking.log_exception(e, issue_id: issue_id, project_id: project_id)
+    IssueRebalancingWorker.perform_async(nil, project_id.presence || issue.project_id)
   end
   # rubocop: enable CodeReuse/ActiveRecord
+
+  def find_issue(issue_id, project_id)
+    return Issue.id_in(issue_id).first if issue_id
+
+    project = Project.id_in(project_id).first
+    return unless project
+
+    project.issues.first
+  end
 end

changelogs/unreleased/ajk-issue-placement-worker-deduplication.yml

+---
+title: Pass project ID to issue placement worker
+merge_request: 42091
+author:
+type: performance

spec/services/issues/create_service_spec.rb

@@ -119,7 +119,7 @@ RSpec.describe Issues::CreateService do
       end
 
       it 'moves the issue to the end, in an asynchronous worker' do
-        expect(IssuePlacementWorker).to receive(:perform_async).with(Integer)
+        expect(IssuePlacementWorker).to receive(:perform_async).with(be_nil, Integer)
 
         described_class.new(project, user, opts).execute
       end

spec/workers/issue_placement_worker_spec.rb

@@ -9,83 +9,124 @@ RSpec.describe IssuePlacementWorker do
     let_it_be(:author) { create(:user) }
     let_it_be(:common_attrs) { { author: author, project: project } }
     let_it_be(:unplaced) { common_attrs.merge(relative_position: nil) }
-    let_it_be(:issue) { create(:issue, **unplaced, created_at: time) }
-    let_it_be(:issue_a) { create(:issue, **unplaced, created_at: time - 1.minute) }
-    let_it_be(:issue_b) { create(:issue, **unplaced, created_at: time - 2.minutes) }
-    let_it_be(:issue_c) { create(:issue, **unplaced, created_at: time + 1.minute) }
-    let_it_be(:issue_d) { create(:issue, **unplaced, created_at: time + 2.minutes) }
-    let_it_be(:issue_e) { create(:issue, **common_attrs, relative_position: 10, created_at: time + 1.minute) }
-    let_it_be(:issue_f) { create(:issue, **unplaced, created_at: time + 1.minute) }
+    let_it_be_with_reload(:issue) { create(:issue, **unplaced, created_at: time) }
+    let_it_be_with_reload(:issue_a) { create(:issue, **unplaced, created_at: time - 1.minute) }
+    let_it_be_with_reload(:issue_b) { create(:issue, **unplaced, created_at: time - 2.minutes) }
+    let_it_be_with_reload(:issue_c) { create(:issue, **unplaced, created_at: time + 1.minute) }
+    let_it_be_with_reload(:issue_d) { create(:issue, **unplaced, created_at: time + 2.minutes) }
+    let_it_be_with_reload(:issue_e) { create(:issue, **common_attrs, relative_position: 10, created_at: time + 1.minute) }
+    let_it_be_with_reload(:issue_f) { create(:issue, **unplaced, created_at: time + 1.minute) }
 
     let_it_be(:irrelevant) { create(:issue, relative_position: nil, created_at: time) }
 
-    it 'places all issues created at most 5 minutes before this one at the end, most recent last' do
-      expect do
-        described_class.new.perform(issue.id)
-      end.not_to change { irrelevant.reset.relative_position }
+    shared_examples 'running the issue placement worker' do
+      let(:issue_id) { issue.id }
+      let(:project_id) { project.id }
 
-      expect(project.issues.order_relative_position_asc)
-        .to eq([issue_e, issue_b, issue_a, issue, issue_c, issue_f, issue_d])
-      expect(project.issues.where(relative_position: nil)).not_to exist
-    end
+      it 'places all issues created at most 5 minutes before this one at the end, most recent last' do
+        expect { run_worker }.not_to change { irrelevant.reset.relative_position }
 
-    it 'schedules rebalancing if needed' do
-      issue_a.update!(relative_position: RelativePositioning::MAX_POSITION)
+        expect(project.issues.order_relative_position_asc)
+          .to eq([issue_e, issue_b, issue_a, issue, issue_c, issue_f, issue_d])
+        expect(project.issues.where(relative_position: nil)).not_to exist
+      end
 
-      expect(IssueRebalancingWorker).to receive(:perform_async).with(nil, project.id)
+      it 'schedules rebalancing if needed' do
+        issue_a.update!(relative_position: RelativePositioning::MAX_POSITION)
 
-      described_class.new.perform(issue.id)
-    end
+        expect(IssueRebalancingWorker).to receive(:perform_async).with(nil, project.id)
 
-    context 'there are more than QUERY_LIMIT unplaced issues' do
-      before_all do
-        # Ensure there are more than N issues in this set
-        n = described_class::QUERY_LIMIT
-        create_list(:issue, n - 5, **unplaced)
+        run_worker
       end
 
-      it 'limits the sweep to QUERY_LIMIT records, and reschedules placement' do
-        expect(Issue).to receive(:move_nulls_to_end)
-                           .with(have_attributes(count: described_class::QUERY_LIMIT))
-                           .and_call_original
+      context 'there are more than QUERY_LIMIT unplaced issues' do
+        before_all do
+          # Ensure there are more than N issues in this set
+          n = described_class::QUERY_LIMIT
+          create_list(:issue, n - 5, **unplaced)
+        end
+
+        it 'limits the sweep to QUERY_LIMIT records, and reschedules placement' do
+          expect(Issue).to receive(:move_nulls_to_end)
+                             .with(have_attributes(count: described_class::QUERY_LIMIT))
+                             .and_call_original
+
+          expect(described_class).to receive(:perform_async).with(nil, project.id)
+
+          run_worker
+
+          expect(project.issues.where(relative_position: nil)).to exist
+        end
+
+        it 'is eventually correct' do
+          prefix = project.issues.where.not(relative_position: nil).order(:relative_position).to_a
+          moved = project.issues.where.not(id: prefix.map(&:id))
 
-        expect(described_class).to receive(:perform_async).with(issue_d.id)
+          run_worker
 
-        described_class.new.perform(issue.id)
+          expect(project.issues.where(relative_position: nil)).to exist
 
-        expect(project.issues.where(relative_position: nil)).to exist
+          run_worker
+
+          expect(project.issues.where(relative_position: nil)).not_to exist
+          expect(project.issues.order(:relative_position)).to eq(prefix + moved.order(:created_at, :id))
+        end
       end
 
-      it 'is eventually correct' do
-        prefix = project.issues.where.not(relative_position: nil).order(:relative_position).to_a
-        moved = project.issues.where.not(id: prefix.map(&:id))
+      context 'we are passed bad IDs' do
+        let(:issue_id) { non_existing_record_id }
+        let(:project_id) { non_existing_record_id }
 
-        described_class.new.perform(issue.id)
+        def max_positions_by_project
+          Issue
+            .group(:project_id)
+            .pluck(:project_id, Issue.arel_table[:relative_position].maximum.as('max_relative_position'))
+            .to_h
+        end
 
-        expect(project.issues.where(relative_position: nil)).to exist
+        it 'does move any issues to the end' do
+          expect { run_worker }.not_to change { max_positions_by_project }
+        end
 
-        described_class.new.perform(issue.id)
+        context 'the project_id refers to an empty project' do
+          let!(:project_id) { create(:project).id }
 
-        expect(project.issues.where(relative_position: nil)).not_to exist
-        expect(project.issues.order(:relative_position)).to eq(prefix + moved.order(:created_at, :id))
+          it 'does move any issues to the end' do
+            expect { run_worker }.not_to change { max_positions_by_project }
+          end
+        end
+      end
+
+      it 'anticipates the failure to place the issues, and schedules rebalancing' do
+        allow(Issue).to receive(:move_nulls_to_end) { raise RelativePositioning::NoSpaceLeft }
+
+        expect(IssueRebalancingWorker).to receive(:perform_async).with(nil, project.id)
+        expect(Gitlab::ErrorTracking)
+          .to receive(:log_exception)
+          .with(RelativePositioning::NoSpaceLeft, worker_arguments)
+
+        run_worker
       end
     end
 
-    it 'anticipates the failure to find the issue' do
-      id = non_existing_record_id
+    context 'passing an issue ID' do
+      def run_worker
+        described_class.new.perform(issue_id)
+      end
+
+      let(:worker_arguments) { { issue_id: issue_id, project_id: nil } }
 
-      expect { described_class.new.perform(id) }.not_to raise_error
+      it_behaves_like 'running the issue placement worker'
     end
 
-    it 'anticipates the failure to place the issues, and schedules rebalancing' do
-      allow(Issue).to receive(:move_nulls_to_end) { raise RelativePositioning::NoSpaceLeft }
+    context 'passing a project ID' do
+      def run_worker
+        described_class.new.perform(nil, project_id)
+      end
 
-      expect(IssueRebalancingWorker).to receive(:perform_async).with(nil, project.id)
-      expect(Gitlab::ErrorTracking)
-        .to receive(:log_exception)
-        .with(RelativePositioning::NoSpaceLeft, issue_id: issue.id)
+      let(:worker_arguments) { { issue_id: nil, project_id: project_id } }
 
-      described_class.new.perform(issue.id)
+      it_behaves_like 'running the issue placement worker'
     end
   end
 end