Merge branch 'ajk-relative-positioning-worker' into 'master'

[RelativePositioning] Add rebalancing worker for issues

See merge request gitlab-org/gitlab!40124

app/services/issue_rebalancing_service.rb

+# frozen_string_literal: true
+
+class IssueRebalancingService
+  MAX_ISSUE_COUNT = 10_000
+  TooManyIssues = Class.new(StandardError)
+
+  def initialize(issue)
+    @issue = issue
+    @base = Issue.relative_positioning_query_base(issue)
+  end
+
+  def execute
+    gates = [issue.project, issue.project.group].compact
+    return unless gates.any? { |gate| Feature.enabled?(:rebalance_issues, gate) }
+
+    raise TooManyIssues, "#{issue_count} issues" if issue_count > MAX_ISSUE_COUNT
+
+    start = RelativePositioning::START_POSITION - (gaps / 2) * gap_size
+
+    Issue.transaction do
+      indexed_ids.each_slice(100) { |pairs| assign_positions(start, pairs) }
+    end
+  end
+
+  private
+
+  attr_reader :issue, :base
+
+  # rubocop: disable CodeReuse/ActiveRecord
+  def indexed_ids
+    base.reorder(:relative_position, :id).pluck(:id).each_with_index
+  end
+  # rubocop: enable CodeReuse/ActiveRecord
+
+  # rubocop: disable CodeReuse/ActiveRecord
+  def assign_positions(start, positions)
+    values = positions.map do |id, index|
+      "(#{id}, #{start + (index * gap_size)})"
+    end.join(', ')
+
+    Issue.connection.exec_query(<<~SQL, "rebalance issue positions")
+      WITH cte(cte_id, new_pos) AS (
+       SELECT *
+       FROM (VALUES #{values}) as t (id, pos)
+      )
+      UPDATE #{Issue.table_name}
+      SET relative_position = cte.new_pos
+      FROM cte
+      WHERE cte_id = id
+    SQL
+  end
+  # rubocop: enable CodeReuse/ActiveRecord
+
+  def issue_count
+    @issue_count ||= base.count
+  end
+
+  def gaps
+    issue_count - 1
+  end
+
+  def gap_size
+    # We could try to split the available range over the number of gaps we need,
+    # but IDEAL_DISTANCE * MAX_ISSUE_COUNT is only 0.1% of the available range,
+    # so we are guaranteed not to exhaust it by using this static value.
+    #
+    # If we raise MAX_ISSUE_COUNT or IDEAL_DISTANCE significantly, this may
+    # change!
+    RelativePositioning::IDEAL_DISTANCE
+  end
+end

app/services/issues/base_service.rb

@@ -19,6 +19,19 @@ module Issues
 
     private
 
+    NO_REBALANCING_NEEDED = ((RelativePositioning::MIN_POSITION * 0.9999)..(RelativePositioning::MAX_POSITION * 0.9999)).freeze
+
+    def rebalance_if_needed(issue)
+      return unless issue
+      return if issue.relative_position.nil?
+      return if NO_REBALANCING_NEEDED.cover?(issue.relative_position)
+
+      gates = [issue.project, issue.project.group].compact
+      return unless gates.any? { |gate| Feature.enabled?(:rebalance_issues, gate) }
+
+      IssueRebalancingWorker.perform_async(issue.id)
+    end
+
     def create_assignee_note(issue, old_assignees)
       SystemNoteService.change_issuable_assignees(
         issue, issue.project, current_user, old_assignees)

app/services/issues/create_service.rb

@@ -30,6 +30,7 @@ module Issues
       user_agent_detail_service.create
       resolve_discussions_with_issue(issuable)
       delete_milestone_total_issue_counter_cache(issuable.milestone)
+      rebalance_if_needed(issuable)
 
       super
     end

app/services/issues/update_service.rb

@@ -83,6 +83,7 @@ module Issues
       raise ActiveRecord::RecordNotFound unless issue_before || issue_after
 
       issue.move_between(issue_before, issue_after)
+      rebalance_if_needed(issue)
     end
 
     # rubocop: disable CodeReuse/ActiveRecord

app/workers/issue_rebalancing_worker.rb

+# frozen_string_literal: true
+
+class IssueRebalancingWorker
+  include ApplicationWorker
+
+  idempotent!
+  urgency :low
+  feature_category :issue_tracking
+
+  def perform(issue_id)
+    issue = Issue.find(issue_id)
+
+    IssueRebalancingService.new(issue).execute
+  rescue ActiveRecord::RecordNotFound, IssueRebalancingService::TooManyIssues => e
+    Gitlab::ErrorTracking.log_exception(e, issue_id: issue_id)
+  end
+end

changelogs/unreleased/ajk-relative-positioning-worker.yml

+---
+title: Add background worker to rebalance issues
+merge_request: 40124
+author:
+type: added

config/feature_flags/development/rebalance_issues.yml

+---
+name: rebalance_issues
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/40124 
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/239344
+group: 'group::project management'
+type: development
+default_enabled: false

config/sidekiq_queues.yml

@@ -138,6 +138,8 @@
   - 2
 - - irker
   - 1
+- - issue_rebalancing
+  - 1
 - - jira_connect
   - 1
 - - jira_importer

spec/services/issue_rebalancing_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe IssueRebalancingService do
+  let_it_be(:project) { create(:project) }
+  let_it_be(:user) { project.creator }
+  let_it_be(:start) { RelativePositioning::START_POSITION }
+  let_it_be(:max_pos) { RelativePositioning::MAX_POSITION }
+  let_it_be(:min_pos) { RelativePositioning::MIN_POSITION }
+  let_it_be(:clump_size) { 300 }
+
+  let_it_be(:unclumped) do
+    (0..clump_size).to_a.map do |i|
+      create(:issue, project: project, author: user, relative_position: start + (1024 * i))
+    end
+  end
+
+  let_it_be(:end_clump) do
+    (0..clump_size).to_a.map do |i|
+      create(:issue, project: project, author: user, relative_position: max_pos - i)
+    end
+  end
+
+  let_it_be(:start_clump) do
+    (0..clump_size).to_a.map do |i|
+      create(:issue, project: project, author: user, relative_position: min_pos + i)
+    end
+  end
+
+  def issues_in_position_order
+    project.reload.issues.reorder(relative_position: :asc).to_a
+  end
+
+  it 'rebalances a set of issues with clumps at the end and start' do
+    all_issues = start_clump + unclumped + end_clump.reverse
+    service = described_class.new(project.issues.first)
+
+    expect { service.execute }.not_to change { issues_in_position_order.map(&:id) }
+
+    all_issues.each(&:reset)
+
+    gaps = all_issues.take(all_issues.count - 1).zip(all_issues.drop(1)).map do |a, b|
+      b.relative_position - a.relative_position
+    end
+
+    expect(gaps).to all(be > RelativePositioning::MIN_GAP)
+    expect(all_issues.first.relative_position).to be > (RelativePositioning::MIN_POSITION * 0.9999)
+    expect(all_issues.last.relative_position).to be < (RelativePositioning::MAX_POSITION * 0.9999)
+  end
+
+  it 'is idempotent' do
+    service = described_class.new(project.issues.first)
+
+    expect do
+      service.execute
+      service.execute
+    end.not_to change { issues_in_position_order.map(&:id) }
+  end
+
+  it 'does nothing if the feature flag is disabled' do
+    stub_feature_flags(rebalance_issues: false)
+    issue = project.issues.first
+    issue.project
+    issue.project.group
+    old_pos = issue.relative_position
+
+    service = described_class.new(issue)
+
+    expect { service.execute }.not_to exceed_query_limit(0)
+    expect(old_pos).to eq(issue.reload.relative_position)
+  end
+
+  it 'acts if the flag is enabled for the project' do
+    issue = create(:issue, project: project, author: user, relative_position: max_pos)
+    stub_feature_flags(rebalance_issues: issue.project)
+
+    service = described_class.new(issue)
+
+    expect { service.execute }.to change { issue.reload.relative_position }
+  end
+
+  it 'acts if the flag is enabled for the group' do
+    issue = create(:issue, project: project, author: user, relative_position: max_pos)
+    project.update!(group: create(:group))
+    stub_feature_flags(rebalance_issues: issue.project.group)
+
+    service = described_class.new(issue)
+
+    expect { service.execute }.to change { issue.reload.relative_position }
+  end
+
+  it 'aborts if there are too many issues' do
+    issue = project.issues.first
+    base = double(count: 10_001)
+
+    allow(Issue).to receive(:relative_positioning_query_base).with(issue).and_return(base)
+
+    expect { described_class.new(issue).execute }.to raise_error(described_class::TooManyIssues)
+  end
+end

spec/services/issues/create_service_spec.rb

@@ -75,6 +75,37 @@ RSpec.describe Issues::CreateService do
         expect(Todo.where(attributes).count).to eq 1
       end
 
+      it 'rebalances if needed' do
+        create(:issue, project: project, relative_position: RelativePositioning::MAX_POSITION)
+        expect(IssueRebalancingWorker).to receive(:perform_async).with(Integer)
+
+        expect(issue.relative_position).to eq(project.issues.maximum(:relative_position))
+      end
+
+      it 'does not rebalance if the flag is disabled' do
+        stub_feature_flags(rebalance_issues: false)
+
+        create(:issue, project: project, relative_position: RelativePositioning::MAX_POSITION)
+        expect(IssueRebalancingWorker).not_to receive(:perform_async).with(Integer)
+
+        expect(issue.relative_position).to eq(project.issues.maximum(:relative_position))
+      end
+
+      it 'does rebalance if the flag is enabled for the project' do
+        stub_feature_flags(rebalance_issues: project)
+
+        create(:issue, project: project, relative_position: RelativePositioning::MAX_POSITION)
+        expect(IssueRebalancingWorker).to receive(:perform_async).with(Integer)
+
+        expect(issue.relative_position).to eq(project.issues.maximum(:relative_position))
+      end
+
+      it 'does not rebalance unless needed' do
+        expect(IssueRebalancingWorker).not_to receive(:perform_async)
+
+        expect(issue.relative_position).to eq(project.issues.maximum(:relative_position))
+      end
+
       context 'when label belongs to project group' do
         let(:group) { create(:group) }
         let(:group_labels) { create_pair(:group_label, group: group) }

spec/services/issues/update_service_spec.rb

@@ -106,7 +106,7 @@ RSpec.describe Issues::UpdateService, :mailer do
 
         [issue, issue1, issue2].each do |issue|
           issue.move_to_end
-          issue.save
+          issue.save!
         end
 
         opts[:move_between_ids] = [issue1.id, issue2.id]
@@ -116,6 +116,66 @@ RSpec.describe Issues::UpdateService, :mailer do
         expect(issue.relative_position).to be_between(issue1.relative_position, issue2.relative_position)
       end
 
+      it 'does not rebalance even if needed if the flag is disabled' do
+        stub_feature_flags(rebalance_issues: false)
+
+        range = described_class::NO_REBALANCING_NEEDED
+        issue1 = create(:issue, project: project, relative_position: range.first - 100)
+        issue2 = create(:issue, project: project, relative_position: range.first)
+        issue.update!(relative_position: RelativePositioning::START_POSITION)
+
+        opts[:move_between_ids] = [issue1.id, issue2.id]
+
+        expect(IssueRebalancingWorker).not_to receive(:perform_async).with(issue.id)
+
+        update_issue(opts)
+        expect(issue.relative_position).to be_between(issue1.relative_position, issue2.relative_position)
+      end
+
+      it 'rebalances if needed if the flag is enabled for the project' do
+        stub_feature_flags(rebalance_issues: project)
+
+        range = described_class::NO_REBALANCING_NEEDED
+        issue1 = create(:issue, project: project, relative_position: range.first - 100)
+        issue2 = create(:issue, project: project, relative_position: range.first)
+        issue.update!(relative_position: RelativePositioning::START_POSITION)
+
+        opts[:move_between_ids] = [issue1.id, issue2.id]
+
+        expect(IssueRebalancingWorker).to receive(:perform_async).with(issue.id)
+
+        update_issue(opts)
+        expect(issue.relative_position).to be_between(issue1.relative_position, issue2.relative_position)
+      end
+
+      it 'rebalances if needed on the left' do
+        range = described_class::NO_REBALANCING_NEEDED
+        issue1 = create(:issue, project: project, relative_position: range.first - 100)
+        issue2 = create(:issue, project: project, relative_position: range.first)
+        issue.update!(relative_position: RelativePositioning::START_POSITION)
+
+        opts[:move_between_ids] = [issue1.id, issue2.id]
+
+        expect(IssueRebalancingWorker).to receive(:perform_async).with(issue.id)
+
+        update_issue(opts)
+        expect(issue.relative_position).to be_between(issue1.relative_position, issue2.relative_position)
+      end
+
+      it 'rebalances if needed on the right' do
+        range = described_class::NO_REBALANCING_NEEDED
+        issue1 = create(:issue, project: project, relative_position: range.last)
+        issue2 = create(:issue, project: project, relative_position: range.last + 100)
+        issue.update!(relative_position: RelativePositioning::START_POSITION)
+
+        opts[:move_between_ids] = [issue1.id, issue2.id]
+
+        expect(IssueRebalancingWorker).to receive(:perform_async).with(issue.id)
+
+        update_issue(opts)
+        expect(issue.relative_position).to be_between(issue1.relative_position, issue2.relative_position)
+      end
+
       context 'when moving issue between issues from different projects' do
         let(:group) { create(:group) }
         let(:subgroup) { create(:group, parent: group) }

spec/workers/issue_rebalancing_worker_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe IssueRebalancingWorker do
+  describe '#perform' do
+    let_it_be(:issue) { create(:issue) }
+
+    it 'runs an instance of IssueRebalancingService' do
+      service = double(execute: nil)
+      expect(IssueRebalancingService).to receive(:new).with(issue).and_return(service)
+
+      described_class.new.perform(issue.id)
+    end
+
+    it 'anticipates the inability to find the issue' do
+      expect(Gitlab::ErrorTracking).to receive(:log_exception).with(ActiveRecord::RecordNotFound, include(issue_id: -1))
+      expect(IssueRebalancingService).not_to receive(:new)
+
+      described_class.new.perform(-1)
+    end
+
+    it 'anticipates there being too many issues' do
+      service = double
+      allow(service).to receive(:execute) { raise IssueRebalancingService::TooManyIssues }
+      expect(IssueRebalancingService).to receive(:new).with(issue).and_return(service)
+      expect(Gitlab::ErrorTracking).to receive(:log_exception).with(IssueRebalancingService::TooManyIssues, include(issue_id: issue.id))
+
+      described_class.new.perform(issue.id)
+    end
+  end
+end