From a211d20db988eff21784f752fd615e3a7a789fad Mon Sep 17 00:00:00 2001 From: Thorsten Eckel Date: Tue, 15 Jan 2019 13:32:14 +0100 Subject: [PATCH] Refactoring: Migrated BackgroundJobSearchIndex (Delayed::Job) to SearchIndexJob (ActiveJob). Refactoring: Enable MonitoringController to handle ActiveJob background jobs. --- app/controllers/monitoring_controller.rb | 25 +++++++++++--- .../jobs/search_index_job.rb | 15 +++------ .../concerns/has_search_index_backend.rb | 2 +- spec/jobs/search_index_job_spec.rb | 24 ++++++++++++++ .../has_search_index_backend_examples.rb | 33 +++++++++++++++++++ spec/models/organization_spec.rb | 2 ++ spec/requests/integration/monitoring_spec.rb | 16 ++++----- 7 files changed, 92 insertions(+), 25 deletions(-) rename lib/background_job_search_index.rb => app/jobs/search_index_job.rb (64%) create mode 100644 spec/jobs/search_index_job_spec.rb create mode 100644 spec/models/concerns/has_search_index_backend_examples.rb diff --git a/app/controllers/monitoring_controller.rb b/app/controllers/monitoring_controller.rb index 7d4aa3172..8f2ee12b4 100644 --- a/app/controllers/monitoring_controller.rb +++ b/app/controllers/monitoring_controller.rb @@ -99,11 +99,26 @@ curl http://localhost/api/v1/monitoring/health_check?token=XXX issues.push "#{count_failed_jobs} failing background jobs" end - listed_failed_jobs = failed_jobs.select(:handler, :attempts).limit(10) - sorted_failed_jobs = listed_failed_jobs.group_by(&:name).sort_by { |_handler, entries| entries.length }.reverse.to_h - sorted_failed_jobs.each_with_index do |(name, jobs), index| - attempts = jobs.map(&:attempts).sum - issues.push "Failed to run background job ##{index += 1} '#{name}' #{jobs.count} time(s) with #{attempts} attempt(s)." + handler_attempts_map = {} + failed_jobs.order(:created_at).limit(10).each do |job| + + job_name = if job.name == 'ActiveJob::QueueAdapters::DelayedJobAdapter::JobWrapper'.freeze + job.payload_object.job_data['job_class'] + else + job.name + end + + handler_attempts_map[job_name] ||= { + count: 0, + attempts: 0, + } + + handler_attempts_map[job_name][:count] += 1 + handler_attempts_map[job_name][:attempts] += job.attempts + end + + Hash[handler_attempts_map.sort].each_with_index do |(job_name, job_data), index| + issues.push "Failed to run background job ##{index + 1} '#{job_name}' #{job_data[:count]} time(s) with #{job_data[:attempts]} attempt(s)." end # job count check diff --git a/lib/background_job_search_index.rb b/app/jobs/search_index_job.rb similarity index 64% rename from lib/background_job_search_index.rb rename to app/jobs/search_index_job.rb index 9fa062efb..96e3c2c64 100644 --- a/lib/background_job_search_index.rb +++ b/app/jobs/search_index_job.rb @@ -1,11 +1,11 @@ -# Copyright (C) 2012-2016 Zammad Foundation, http://zammad-foundation.org/ -class BackgroundJobSearchIndex - def initialize(object, o_id) +class SearchIndexJob < ApplicationJob + + retry_on StandardError, attempts: 20 + + def perform(object, o_id) @object = object @o_id = o_id - end - def perform record = @object.constantize.lookup(id: @o_id) return if !exists?(record) @@ -20,9 +20,4 @@ class BackgroundJobSearchIndex Rails.logger.info "Can't index #{@object}.lookup(id: #{@o_id}), no such record found" false end - - def max_attempts - 20 - end - end diff --git a/app/models/concerns/has_search_index_backend.rb b/app/models/concerns/has_search_index_backend.rb index 09622f472..1f09d77fd 100644 --- a/app/models/concerns/has_search_index_backend.rb +++ b/app/models/concerns/has_search_index_backend.rb @@ -24,7 +24,7 @@ update search index, if configured - will be executed automatically # start background job to transfer data to search index return true if !SearchIndexBackend.enabled? - Delayed::Job.enqueue(BackgroundJobSearchIndex.new(self.class.to_s, id)) + SearchIndexJob.perform_later(self.class.to_s, id) true end diff --git a/spec/jobs/search_index_job_spec.rb b/spec/jobs/search_index_job_spec.rb new file mode 100644 index 000000000..2d037fb64 --- /dev/null +++ b/spec/jobs/search_index_job_spec.rb @@ -0,0 +1,24 @@ +require 'rails_helper' + +RSpec.describe SearchIndexJob, type: :job do + + it 'calls search_index_update_backend on matching record' do + user = create(:user) + expect(::User).to receive(:lookup).with(id: user.id).and_return(user) + expect(user).to receive(:search_index_update_backend) + + described_class.perform_now('User', user.id) + end + + it "doesn't perform for non existing records" do + id = 9999 + expect(::User).to receive(:lookup).with(id: id).and_return(nil) + described_class.perform_now('User', id) + end + + it 'retries on exception' do + expect(::User).to receive(:lookup).and_raise(RuntimeError) + described_class.perform_now('User', 1) + expect(SearchIndexJob).to have_been_enqueued + end +end diff --git a/spec/models/concerns/has_search_index_backend_examples.rb b/spec/models/concerns/has_search_index_backend_examples.rb new file mode 100644 index 000000000..8bb1c6a9e --- /dev/null +++ b/spec/models/concerns/has_search_index_backend_examples.rb @@ -0,0 +1,33 @@ +RSpec.shared_examples 'HasSearchIndexBackend' do |indexed_factory:| + + context '#search_index_update', performs_jobs: true do + subject { create(indexed_factory) } + + before(:each) do + allow(SearchIndexBackend).to receive(:enabled?).and_return(true) + end + + context 'record indexing' do + + before(:each) do + expect(subject).to be_present + end + + it 'indexes on create' do + expect(SearchIndexJob).to have_been_enqueued + end + + it 'indexes on update' do + clear_jobs + subject.update(note: 'Updated') + expect(SearchIndexJob).to have_been_enqueued + end + + it 'indexes on touch' do + clear_jobs + subject.touch + expect(SearchIndexJob).to have_been_enqueued + end + end + end +end diff --git a/spec/models/organization_spec.rb b/spec/models/organization_spec.rb index 1ffce6605..af3718b65 100644 --- a/spec/models/organization_spec.rb +++ b/spec/models/organization_spec.rb @@ -1,8 +1,10 @@ require 'rails_helper' require 'models/concerns/can_lookup_examples' +require 'models/concerns/has_search_index_backend_examples' RSpec.describe Organization do include_examples 'CanLookup' + include_examples 'HasSearchIndexBackend', indexed_factory: :organization context '.where_or_cis' do diff --git a/spec/requests/integration/monitoring_spec.rb b/spec/requests/integration/monitoring_spec.rb index ef24bb0c5..513351901 100644 --- a/spec/requests/integration/monitoring_spec.rb +++ b/spec/requests/integration/monitoring_spec.rb @@ -372,7 +372,6 @@ RSpec.describe 'Monitoring', type: :request do end it 'does check health false' do - channel = Channel.find_by(active: true) channel.status_in = 'ok' channel.status_out = 'error' @@ -423,7 +422,7 @@ RSpec.describe 'Monitoring', type: :request do # health_check - scheduler job count travel 2.seconds 8001.times do - Delayed::Job.enqueue( BackgroundJobSearchIndex.new('Ticket', 1)) + SearchIndexJob.perform_later('Ticket', 1) end Scheduler.where(active: true).each do |local_scheduler| local_scheduler.last_run = Time.zone.now @@ -520,7 +519,6 @@ RSpec.describe 'Monitoring', type: :request do end it 'does check failed delayed job', db_strategy: :reset do - # disable elasticsearch prev_es_config = Setting.get('es_url') Setting.set('es_url', 'http://127.0.0.1:92001') @@ -598,11 +596,11 @@ RSpec.describe 'Monitoring', type: :request do expect(json_response['message']).to be_truthy expect(json_response['issues']).to be_truthy expect(json_response['healthy']).to eq(false) - expect( json_response['message']).to eq("Failed to run background job #1 'BackgroundJobSearchIndex' 1 time(s) with 4 attempt(s).") + expect( json_response['message']).to eq("Failed to run background job #1 'SearchIndexJob' 4 time(s) with 4 attempt(s).") # add another job - manual_added = Delayed::Job.enqueue( BackgroundJobSearchIndex.new('Ticket', 1)) - manual_added.update!(attempts: 10) + manual_added = SearchIndexJob.perform_later('Ticket', 1) + Delayed::Job.find(manual_added.provider_job_id).update!(attempts: 10) # health_check get "/api/v1/monitoring/health_check?token=#{token}", params: {}, as: :json @@ -612,7 +610,7 @@ RSpec.describe 'Monitoring', type: :request do expect(json_response['message']).to be_truthy expect(json_response['issues']).to be_truthy expect(json_response['healthy']).to eq(false) - expect( json_response['message']).to eq("Failed to run background job #1 'BackgroundJobSearchIndex' 2 time(s) with 14 attempt(s).") + expect( json_response['message']).to eq("Failed to run background job #1 'SearchIndexJob' 5 time(s) with 14 attempt(s).") # add another job dummy_class = Class.new do @@ -633,7 +631,7 @@ RSpec.describe 'Monitoring', type: :request do expect(json_response['message']).to be_truthy expect(json_response['issues']).to be_truthy expect(json_response['healthy']).to eq(false) - expect( json_response['message']).to eq("Failed to run background job #1 'BackgroundJobSearchIndex' 2 time(s) with 14 attempt(s).;Failed to run background job #2 'Object' 1 time(s) with 5 attempt(s).") + expect( json_response['message']).to eq("Failed to run background job #1 'Object' 1 time(s) with 5 attempt(s).;Failed to run background job #2 'SearchIndexJob' 5 time(s) with 14 attempt(s).") # reset settings Setting.set('es_url', prev_es_config) @@ -652,7 +650,7 @@ RSpec.describe 'Monitoring', type: :request do expect(json_response['message']).to be_truthy expect(json_response['issues']).to be_truthy expect(json_response['healthy']).to eq(false) - expect( json_response['message']).to eq("13 failing background jobs;Failed to run background job #1 'Object' 8 time(s) with 40 attempt(s).;Failed to run background job #2 'BackgroundJobSearchIndex' 2 time(s) with 14 attempt(s).") + expect(json_response['message']).to eq("16 failing background jobs;Failed to run background job #1 'Object' 5 time(s) with 25 attempt(s).;Failed to run background job #2 'SearchIndexJob' 5 time(s) with 14 attempt(s).") # cleanup Delayed::Job.delete_all