Merge branch 'ajk-relative-positioning-issue-placement-worker-oldest-first' into 'master'

Place older issues before more recent ones

See merge request gitlab-org/gitlab!41602

app/workers/issue_placement_worker.rb

@@ -17,17 +17,21 @@ class IssuePlacementWorker
     issue = Issue.id_in(issue_id).first
     return unless issue
 
-    # Move the most recent 100 unpositioned items to the end.
+    # Move the oldest 100 unpositioned items to the end.
     # This is to deal with out-of-order execution of the worker,
     # while preserving creation order.
     to_place = Issue
       .relative_positioning_query_base(issue)
       .where(relative_position: nil)
-      .order({ created_at: :desc }, { id: :desc })
-      .limit(QUERY_LIMIT)
+      .order({ created_at: :asc }, { id: :asc })
+      .limit(QUERY_LIMIT + 1)
+      .to_a
 
-    Issue.move_nulls_to_end(to_place.to_a.reverse)
+    leftover = to_place.pop if to_place.count > QUERY_LIMIT
+
+    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?
   rescue RelativePositioning::NoSpaceLeft => e
     Gitlab::ErrorTracking.log_exception(e, issue_id: issue_id)
     IssueRebalancingWorker.perform_async(nil, issue.project_id)

changelogs/unreleased/ajk-relative-positioning-issue-placement-worker-oldest-first.yml

+---
+title: Place older issues before more recent ones
+merge_request: 41602
+author:
+type: changed

spec/workers/issue_placement_worker_spec.rb

@@ -37,16 +37,38 @@ RSpec.describe IssuePlacementWorker do
       described_class.new.perform(issue.id)
     end
 
-    it 'limits the sweep to QUERY_LIMIT records' do
-      # Ensure there are more than N issues in this set
-      n = described_class::QUERY_LIMIT
-      create_list(:issue, n - 5, **unplaced)
+    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
 
-      expect(Issue).to receive(:move_nulls_to_end).with(have_attributes(count: n)).and_call_original
+      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
 
-      described_class.new.perform(issue.id)
+        expect(described_class).to receive(:perform_async).with(issue_d.id)
+
+        described_class.new.perform(issue.id)
+
+        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))
+
+        described_class.new.perform(issue.id)
+
+        expect(project.issues.where(relative_position: nil)).to exist
+
+        described_class.new.perform(issue.id)
 
-      expect(project.issues.where(relative_position: nil)).to exist
+        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 'anticipates the failure to find the issue' do