Commit 8d2deef4 authored by Alexandru Croitor's avatar Alexandru Croitor

Skip rebalancing issues with null relative position

When rebalancing issues skip issues that have null position
these will be moved to the end after the rebalance and would
follow the order in which these were created.
parent de909f0a
...@@ -166,6 +166,8 @@ class Issue < ApplicationRecord ...@@ -166,6 +166,8 @@ class Issue < ApplicationRecord
scope :by_project_id_and_iid, ->(composites) do scope :by_project_id_and_iid, ->(composites) do
where_composite(%i[project_id iid], composites) where_composite(%i[project_id iid], composites)
end end
scope :with_null_relative_position, -> { where(relative_position: nil) }
scope :with_non_null_relative_position, -> { where.not(relative_position: nil) }
after_commit :expire_etag_cache, unless: :importing? after_commit :expire_etag_cache, unless: :importing?
after_save :ensure_metrics, unless: :importing? after_save :ensure_metrics, unless: :importing?
......
...@@ -82,7 +82,7 @@ module Issues ...@@ -82,7 +82,7 @@ module Issues
collection.each do |project| collection.each do |project|
caching.cache_current_project_id(project.id) caching.cache_current_project_id(project.id)
index += 1 index += 1
scope = Issue.in_projects(project).order_by_relative_position.select(:id, :relative_position) scope = Issue.in_projects(project).order_by_relative_position.with_non_null_relative_position.select(:id, :relative_position)
with_retry(PREFETCH_ISSUES_BATCH_SIZE, 100) do |batch_size| with_retry(PREFETCH_ISSUES_BATCH_SIZE, 100) do |batch_size|
Gitlab::Pagination::Keyset::Iterator.new(scope: scope).each_batch(of: batch_size) do |batch| Gitlab::Pagination::Keyset::Iterator.new(scope: scope).each_batch(of: batch_size) do |batch|
......
...@@ -31,7 +31,7 @@ class IssuePlacementWorker ...@@ -31,7 +31,7 @@ class IssuePlacementWorker
# while preserving creation order. # while preserving creation order.
to_place = Issue to_place = Issue
.relative_positioning_query_base(issue) .relative_positioning_query_base(issue)
.where(relative_position: nil) .with_null_relative_position
.order({ created_at: :asc }, { id: :asc }) .order({ created_at: :asc }, { id: :asc })
.limit(QUERY_LIMIT + 1) .limit(QUERY_LIMIT + 1)
.to_a .to_a
......
...@@ -3,7 +3,7 @@ ...@@ -3,7 +3,7 @@
require 'spec_helper' require 'spec_helper'
RSpec.describe Issues::RelativePositionRebalancingService, :clean_gitlab_redis_shared_state do RSpec.describe Issues::RelativePositionRebalancingService, :clean_gitlab_redis_shared_state do
let_it_be(:project, reload: true) { create(:project) } let_it_be(:project, reload: true) { create(:project, :repository_disabled, skip_disk_validation: true) }
let_it_be(:user) { project.creator } let_it_be(:user) { project.creator }
let_it_be(:start) { RelativePositioning::START_POSITION } let_it_be(:start) { RelativePositioning::START_POSITION }
let_it_be(:max_pos) { RelativePositioning::MAX_POSITION } let_it_be(:max_pos) { RelativePositioning::MAX_POSITION }
...@@ -28,12 +28,18 @@ RSpec.describe Issues::RelativePositionRebalancingService, :clean_gitlab_redis_s ...@@ -28,12 +28,18 @@ RSpec.describe Issues::RelativePositionRebalancingService, :clean_gitlab_redis_s
end end
end end
let_it_be(:nil_clump, reload: true) do
(1..100).to_a.map do |i|
create(:issue, project: project, author: user, relative_position: nil)
end
end
before do before do
stub_feature_flags(issue_rebalancing_with_retry: false) stub_feature_flags(issue_rebalancing_with_retry: false)
end end
def issues_in_position_order def issues_in_position_order
project.reload.issues.reorder(relative_position: :asc).to_a project.reload.issues.order_by_relative_position.to_a
end end
subject(:service) { described_class.new(Project.id_in(project)) } subject(:service) { described_class.new(Project.id_in(project)) }
...@@ -44,16 +50,19 @@ RSpec.describe Issues::RelativePositionRebalancingService, :clean_gitlab_redis_s ...@@ -44,16 +50,19 @@ RSpec.describe Issues::RelativePositionRebalancingService, :clean_gitlab_redis_s
expect { service.execute }.not_to change { issues_in_position_order.map(&:id) } expect { service.execute }.not_to change { issues_in_position_order.map(&:id) }
caching = service.send(:caching)
all_issues.each(&:reset) all_issues.each(&:reset)
gaps = all_issues.take(all_issues.count - 1).zip(all_issues.drop(1)).map do |a, b| gaps = all_issues.take(all_issues.count - 1).zip(all_issues.drop(1)).map do |a, b|
b.relative_position - a.relative_position b.relative_position - a.relative_position
end end
expect(caching.issue_count).to eq(900)
expect(gaps).to all(be > RelativePositioning::MIN_GAP) expect(gaps).to all(be > RelativePositioning::MIN_GAP)
expect(all_issues.first.relative_position).to be > (RelativePositioning::MIN_POSITION * 0.9999) 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) expect(all_issues.last.relative_position).to be < (RelativePositioning::MAX_POSITION * 0.9999)
expect(project.root_namespace.issue_repositioning_disabled?).to be false expect(project.root_namespace.issue_repositioning_disabled?).to be false
expect(project.issues.with_null_relative_position.count).to eq(100)
end end
it 'is idempotent' do it 'is idempotent' do
...@@ -111,7 +120,7 @@ RSpec.describe Issues::RelativePositionRebalancingService, :clean_gitlab_redis_s ...@@ -111,7 +120,7 @@ RSpec.describe Issues::RelativePositionRebalancingService, :clean_gitlab_redis_s
allow(caching).to receive(:concurrent_running_rebalances_count).and_return(10) allow(caching).to receive(:concurrent_running_rebalances_count).and_return(10)
allow(service).to receive(:caching).and_return(caching) allow(service).to receive(:caching).and_return(caching)
expect { service.execute }.not_to raise_error(Issues::RelativePositionRebalancingService::TooManyConcurrentRebalances) expect { service.execute }.not_to raise_error
end end
context 're-balancing is retried on statement timeout exceptions' do context 're-balancing is retried on statement timeout exceptions' do
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment