Commit e474cdcf authored by Dmitry Gruzd's avatar Dmitry Gruzd Committed by Dylan Griffith

Remove ElasticIndexerWorker changed_fields

We're changing the way indexing works, currently we pass
changed_fields to ElasticIndexerWorker for notes update taking
storage and harming job deduplication. We're replacing
that logic with a separate jobs per each changed note.
parent 74f92cef
......@@ -53,8 +53,7 @@ module Elastic
:update,
self.class.to_s,
self.id,
self.es_id,
changed_fields: self.previous_changes.keys
self.es_id
)
end
......
......@@ -10,6 +10,7 @@ module EE
WEIGHT_ALL = 'Everything'.freeze
WEIGHT_ANY = 'Any'.freeze
WEIGHT_NONE = 'None'.freeze
ELASTICSEARCH_PERMISSION_TRACKED_FIELDS = %w(assignee_ids author_id confidential).freeze
include Elastic::ApplicationVersionedSearch
include UsageStatistics
......@@ -91,6 +92,24 @@ module EE
super if supports_weight?
end
# override
def maintain_elasticsearch_update
super
maintain_elasticsearch_issue_notes_update if elasticsearch_issue_notes_need_updating?
end
def maintain_elasticsearch_issue_notes_update
::Note.searchable.where(noteable: self).find_each do |note|
note.maintain_elasticsearch_update
end
end
def elasticsearch_issue_notes_need_updating?
changed_fields = self.previous_changes.keys
changed_fields && (changed_fields & ELASTICSEARCH_PERMISSION_TRACKED_FIELDS).any?
end
def supports_weight?
project&.feature_available?(:issue_weights)
end
......
......@@ -8,14 +8,9 @@ module EE
end
def update_elasticsearch_index
if issue.project&.use_elasticsearch?
::ElasticIndexerWorker.perform_async(
:update,
'Issue',
issue.id,
issue.es_id,
changed_fields: ['assignee_ids']
)
if issue.project&.use_elasticsearch? && issue.maintaining_elasticsearch?
issue.maintain_elasticsearch_update
issue.maintain_elasticsearch_issue_notes_update # we need to propagate new permissions to notes
end
end
end
......
......@@ -32,8 +32,7 @@ module EE
:update,
project.class.to_s,
project.id,
project.es_id,
changed_fields: ['visibility_level']
project.es_id
)
end
end
......
......@@ -6,7 +6,6 @@ module Elastic
ImportError = Class.new(StandardError)
ISSUE_TRACKED_FIELDS = %w(assignee_ids author_id confidential).freeze
IMPORT_RETRY_COUNT = 3
# @param indexing [Boolean] determines whether operation is "indexing" or "updating"
......@@ -19,8 +18,6 @@ module Elastic
initial_index_project(record) if record.class == Project && indexing
update_issue_notes(record, options["changed_fields"]) if record.class == Issue
true
rescue Elasticsearch::Transport::Transport::Errors::NotFound, ActiveRecord::RecordNotFound => e
# These errors can happen in several cases, including:
......@@ -37,14 +34,6 @@ module Elastic
private
# rubocop: disable CodeReuse/ActiveRecord
def update_issue_notes(record, changed_fields)
if changed_fields && (changed_fields & ISSUE_TRACKED_FIELDS).any?
import_association(Note, query: -> { searchable.where(noteable: record) })
end
end
# rubocop: enable CodeReuse/ActiveRecord
def initial_index_project(project)
# Enqueue the repository indexing jobs immediately so they run in parallel
# One for the project repository, one for the wiki repository
......
......@@ -112,7 +112,7 @@ describe Note, :elastic do
issue = create :issue, project: project, updated_at: 1.minute.ago
# Only issue should be updated
expect(ElasticIndexerWorker).to receive(:perform_async).with(:update, 'Issue', anything, anything, anything)
expect(ElasticIndexerWorker).to receive(:perform_async).with(:update, 'Issue', anything, anything)
create :note, :system, project: project, noteable: issue
end
......
......@@ -305,6 +305,49 @@ describe Issue do
end
end
context 'ES related specs', :elastic do
before do
stub_ee_application_setting(elasticsearch_indexing: true)
end
context 'when updating an Issue' do
let(:project) { create(:project, :public) }
let(:issue) { create(:issue, project: project, confidential: true) }
before do
Sidekiq::Testing.disable! do
create(:note, note: 'the_normal_note', noteable: issue, project: project)
end
Sidekiq::Testing.inline! do
project.maintain_elasticsearch_update
issue.update!(update_field => update_value)
Gitlab::Elastic::Helper.refresh_index
end
end
context 'when changing the confidential value' do
let(:update_field) { :confidential }
let(:update_value) { false }
it 'updates issue notes excluding system notes' do
expect(issue.elasticsearch_issue_notes_need_updating?).to eq(true)
expect(Note.elastic_search('the_normal_note', options: { project_ids: [issue.project.id] }).present?).to eq(true)
end
end
context 'when changing the title' do
let(:update_field) { :title }
let(:update_value) { 'abc' }
it 'does not update issue notes' do
expect(issue.elasticsearch_issue_notes_need_updating?).to eq(false)
expect(Note.elastic_search('the_normal_note', options: { project_ids: [issue.project.id] }).present?).to eq(false)
end
end
end
end
describe 'relative positioning with group boards' do
let(:group) { create(:group) }
let!(:board) { create(:board, group: group) }
......
......@@ -327,51 +327,4 @@ describe Elastic::IndexRecordService, :elastic do
expect(Project.elastic_search('project_1').present?).to eq(false)
end
context 'when updating an Issue' do
context 'when changing the confidential value' do
it 'updates issue notes excluding system notes' do
project = create(:project, :public)
issue = nil
Sidekiq::Testing.disable! do
issue = create(:issue, project: project, confidential: false)
subject.execute(project, true)
subject.execute(issue, false)
create(:note, note: 'the_normal_note', noteable: issue, project: project)
create(:note, note: 'the_system_note', system: true, noteable: issue, project: project)
end
options = { project_ids: [project.id] }
Sidekiq::Testing.inline! do
expect(subject.execute(issue, false, 'changed_fields' => ['confidential'])).to eq(true)
Gitlab::Elastic::Helper.refresh_index
end
expect(Note.elastic_search('the_normal_note', options: options).present?).to eq(true)
expect(Note.elastic_search('the_system_note', options: options).present?).to eq(false)
end
end
context 'when changing the title' do
it 'does not update issue notes' do
issue = nil
Sidekiq::Testing.disable! do
issue = create(:issue, confidential: false)
subject.execute(issue.project, true)
subject.execute(issue, false)
create(:note, note: 'the_normal_note', noteable: issue, project: issue.project)
end
options = { project_ids: [issue.project.id] }
Sidekiq::Testing.inline! do
expect(subject.execute(issue, false, 'changed_fields' => ['title'])).to eq(true)
Gitlab::Elastic::Helper.refresh_index
end
expect(Note.elastic_search('the_normal_note', options: options).present?).to eq(false)
end
end
end
end
......@@ -85,11 +85,11 @@ describe Groups::TransferService, '#execute' do
project3 = create(:project, :repository, :private, namespace: group)
expect(ElasticIndexerWorker).to receive(:perform_async)
.with(:update, "Project", project1.id, project1.es_id, changed_fields: array_including('visibility_level'))
.with(:update, "Project", project1.id, project1.es_id)
expect(ElasticIndexerWorker).to receive(:perform_async)
.with(:update, "Project", project2.id, project2.es_id, changed_fields: array_including('visibility_level'))
.with(:update, "Project", project2.id, project2.es_id)
expect(ElasticIndexerWorker).not_to receive(:perform_async)
.with(:update, "Project", project3.id, project3.es_id, changed_fields: array_including('visibility_level'))
.with(:update, "Project", project3.id, project3.es_id)
transfer_service.execute(new_group)
......
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