From 63716015cd613cdc06739461a0bff3bdf4d87133 Mon Sep 17 00:00:00 2001 From: Adam Stankiewicz Date: Wed, 18 Feb 2015 14:39:58 -0800 Subject: [PATCH] Introduce concept of Failed Jobs --- Gemfile | 3 +- app/controllers/components_controller.rb | 6 --- app/controllers/main_controller.rb | 8 +-- app/models/build/converter.rb | 50 ++++--------------- app/models/failed_job.rb | 7 +++ app/models/version.rb | 7 +-- app/views/main/status.slim | 27 ++-------- app/workers/build_version.rb | 17 ++++++- app/workers/update_component.rb | 3 +- app/workers/update_scheduler.rb | 2 +- bin/clean_all | 1 + bin/foreman | 16 ++++++ .../20150218105340_create_failed_jobs.rb | 13 +++++ db/schema.rb | 12 ++++- 14 files changed, 88 insertions(+), 84 deletions(-) create mode 100644 app/models/failed_job.rb create mode 100755 bin/foreman create mode 100644 db/migrate/20150218105340_create_failed_jobs.rb diff --git a/Gemfile b/Gemfile index b063685e..96193d4e 100644 --- a/Gemfile +++ b/Gemfile @@ -54,10 +54,11 @@ gem 'whenever' gem 'postrank-uri' +gem 'foreman' + group :development do gem 'better_errors' gem 'binding_of_caller' - gem 'foreman' end group :development, :test do diff --git a/app/controllers/components_controller.rb b/app/controllers/components_controller.rb index b8a963d1..dba43539 100644 --- a/app/controllers/components_controller.rb +++ b/app/controllers/components_controller.rb @@ -19,12 +19,6 @@ def create name, version = component_params[:name], component_params[:version] name = Build::Utils.fix_gem_name(name, version).gsub('/', '--') - component, ver = get_version(name, version) - - if ver.present? && ver.build_status == "failed" - ver.destroy - end - Build::Converter.run!(name, version) component, ver = get_version(name, version) diff --git a/app/controllers/main_controller.rb b/app/controllers/main_controller.rb index 41513ab8..1cc0f260 100644 --- a/app/controllers/main_controller.rb +++ b/app/controllers/main_controller.rb @@ -9,9 +9,11 @@ def home def status @pending_index = Version.includes(:component).pending_index.load - @failed_builds = Version.includes(:component).failed.load - @pending_builds = Sidekiq::Queue.new("default").map(&:as_json).map { |i| i["item"]["args"] } - @failed_jobs = Sidekiq.redis { |c| c.lrange(:failed, 0, 50) }.map { |j| JSON.parse(j) } + + @pending_builds = Sidekiq::Queue.new("default").map(&:as_json) + .map { |i| i["item"]["args"] } + + @failed_jobs = FailedJob.all.to_a end def dependencies diff --git a/app/models/build/converter.rb b/app/models/build/converter.rb index ec55c6ad..639f544d 100644 --- a/app/models/build/converter.rb +++ b/app/models/build/converter.rb @@ -22,21 +22,11 @@ def run!(name, version = nil) # Lock here prevents multiple builds on same requests at the same time Build::Locking.with_lock(lock_name) do - # TODO: should component be saved if some of the dependencies failed? Converter.process!(name, version) do |versions_paths| Converter.persist!(versions_paths) - ::Reindex.perform_async + Reindex.perform_async - versions = versions_paths.map(&:first).compact - - if versions.any? { |v| v.failed? } - raise BuildError.new( - versions.select { |v| v.failed? }. - map { |v| "#{v.component.name}##{v.string}: #{v.build_message}" }.join("\n") - ) - end - - versions.first + versions_paths.first.first end end end @@ -48,7 +38,8 @@ def run!(name, version = nil) # # Yields [Array of Version, Array of Path] # - paths to builded .gem files to insert into index - # - versions to save with build_status set to "builded" or "failed" + # (if path is nil, then component has been alredy built) + # - versions to save with build_status set to "builded" # - build_message and asset_paths + main_paths filled # - no version is yet persisted # @@ -62,42 +53,21 @@ def process!(component_name, component_version = nil) components.map do |component| version = component.version_model - next [version, nil] unless version.needs_build? - ::Rails.logger.info "Building #{component.name}##{version.string}..." - gem_path = begin - Converter.convert!(component) do |output_dir, asset_paths, main_paths| - version.build_status = 'builded' - version.asset_paths = asset_paths.map(&:to_s) - version.main_paths = main_paths.map(&:to_s) - Converter.build!(component, output_dir, gems_dir) - end - rescue Build::BuildError => e - Raven.capture_exception(e) - version.build_status = 'failed' - version.build_message = e.message - nil + gem_filepath = Converter.convert!(component) do |output_dir, asset_paths, main_paths| + version.build_status = 'builded' + version.asset_paths = asset_paths.map(&:to_s) + version.main_paths = main_paths.map(&:to_s) + Converter.build!(component, output_dir, gems_dir) end - [version, gem_path] + [version, gem_filepath] end.compact end yield results end - rescue Build::BowerError, Build::BuildError => e - gem_name = Build::Utils.fix_gem_name(component_name, component_version) - gem_version = Build::Utils.fix_version_string(component_version) - component, version = Component.get(gem_name, gem_version) - - if version - version.build_status = 'failed' - version.build_message = e.message - version.save! - end - - raise end # Public: Persists versions and .gem files returned from convert! method diff --git a/app/models/failed_job.rb b/app/models/failed_job.rb new file mode 100644 index 00000000..8db58c16 --- /dev/null +++ b/app/models/failed_job.rb @@ -0,0 +1,7 @@ +class FailedJob < ActiveRecord::Base + serialize :args, Array + + def retry! + klass.constantize.perform_async(args) + end +end diff --git a/app/models/version.rb b/app/models/version.rb index 4660336e..0dda943c 100644 --- a/app/models/version.rb +++ b/app/models/version.rb @@ -12,10 +12,9 @@ class Version < ActiveRecord::Base scope :indexed, lambda { where(:build_status => "indexed") } scope :builded, lambda { where(:build_status => ["builded", "indexed"]) } scope :pending_index, lambda { where(:build_status => "builded") } - scope :failed, lambda { where(:build_status => "failed") } scope :processed, lambda { - where(build_status: ["builded", "indexed", "failed"], rebuild: false) + where(build_status: ["builded", "indexed"], rebuild: false) } scope :string, lambda { |string| @@ -70,10 +69,6 @@ def builded? build_status == 'builded' || build_status == 'indexed' end - def failed? - build_status == 'failed' - end - def needs_build? (build_status != 'builded' && build_status != 'indexed') || rebuild? end diff --git a/app/views/main/status.slim b/app/views/main/status.slim index 3b8854c3..3b398a57 100644 --- a/app/views/main/status.slim +++ b/app/views/main/status.slim @@ -18,35 +18,16 @@ article.section li = args.join('#') article.failed-builds style="margin-top: 2em" - h1 Permanently failed builds: + h1 Failed builds: - if @failed_builds.empty? | None - else table style="width: 100%" thead - th.unit-1-4 Component + th.unit-1-4 ID th.unit-3-4 Error - tbody - - @failed_builds.each do |version| - tr - td = "#{version.component.name}##{version.string}" - td: pre = version.build_message - - article.failed-builds style="margin-top: 2em" - h1 Failed jobs (last 50) - - if @failed_jobs.empty? - | None - - else - table style="width: 100%" - thead - th.unit-1-4 Job Type - th.unit-3-4 Error Message tbody - @failed_jobs.each do |job| tr - td - = job["payload"]["class"] - br - = job["payload"]["args"].join(", ") - - td = job["error"] + td = job.name + td: pre = job.message diff --git a/app/workers/build_version.rb b/app/workers/build_version.rb index 8b408407..9efca143 100644 --- a/app/workers/build_version.rb +++ b/app/workers/build_version.rb @@ -1,10 +1,23 @@ class BuildVersion include Sidekiq::Worker - sidekiq_options queue: 'default', unique: :all, retry: 0 + sidekiq_options queue: 'default', unique: :all, retry: 3 + + sidekiq_retry_in do |count| + (count ** 5) + 60 + (rand(30) * (count + 1)) + end + + sidekiq_retries_exhausted do |msg| + FailedJob.find_or_create_by(name: msg['args'].join('#')) do |j| + j.worker = msg['class'] + j.args = msg['args'] + j.message = msg['error_message'] + end + end def perform(bower_name, version) - Rails.logger.info "Building #{bower_name}##{version}..." + return if FailedJob.exists?(name: "#{bower_name}##{version}") + Build::Converter.run!(bower_name, version) end end diff --git a/app/workers/update_component.rb b/app/workers/update_component.rb index c602da3c..abc181d9 100644 --- a/app/workers/update_component.rb +++ b/app/workers/update_component.rb @@ -1,7 +1,8 @@ class UpdateComponent include Sidekiq::Worker - sidekiq_options queue: 'update_component', unique: :all, retry: 0 + # We don't want to retry this kind of job. It's automatically retried by scheduler. + sidekiq_options queue: 'update_component', unique: :all, retry: false def perform(bower_name) diff --git a/app/workers/update_scheduler.rb b/app/workers/update_scheduler.rb index bb5afb34..38f8f58a 100644 --- a/app/workers/update_scheduler.rb +++ b/app/workers/update_scheduler.rb @@ -1,7 +1,7 @@ class UpdateScheduler include Sidekiq::Worker - sidekiq_options :queue => 'update_scheduler' + sidekiq_options :queue => 'update_scheduler', unique: :all, retry: false def perform Component.select(:id, :bower_name).find_each do |component| diff --git a/bin/clean_all b/bin/clean_all index e72817a3..03aa8e3b 100755 --- a/bin/clean_all +++ b/bin/clean_all @@ -7,3 +7,4 @@ rm -rf public/gems public/quick rm -f public/*.gz public/*4.8 public/*.lock rake db:drop db:create db:migrate redis-cli FLUSHALL +rake db:test:prepare diff --git a/bin/foreman b/bin/foreman new file mode 100755 index 00000000..586c07e0 --- /dev/null +++ b/bin/foreman @@ -0,0 +1,16 @@ +#!/usr/bin/env ruby +# +# This file was generated by Bundler. +# +# The application 'foreman' is installed as part of a gem, and +# this file is here to facilitate running it. +# + +require 'pathname' +ENV['BUNDLE_GEMFILE'] ||= File.expand_path("../../Gemfile", + Pathname.new(__FILE__).realpath) + +require 'rubygems' +require 'bundler/setup' + +load Gem.bin_path('foreman', 'foreman') diff --git a/db/migrate/20150218105340_create_failed_jobs.rb b/db/migrate/20150218105340_create_failed_jobs.rb new file mode 100644 index 00000000..7f93f72d --- /dev/null +++ b/db/migrate/20150218105340_create_failed_jobs.rb @@ -0,0 +1,13 @@ +class CreateFailedJobs < ActiveRecord::Migration + def change + create_table :failed_jobs do |t| + t.string :name + t.string :worker + t.text :args + t.text :message + + t.timestamps + end + add_index :failed_jobs, :name + end +end diff --git a/db/schema.rb b/db/schema.rb index 3c417cd9..55d8952a 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20140713225251) do +ActiveRecord::Schema.define(version: 20150218105340) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -27,6 +27,16 @@ t.index ["name"], :name => "index_components_on_name", :unique => true, :order => {"name" => :asc} end + create_table "failed_jobs", force: true do |t| + t.string "name" + t.string "worker" + t.text "args" + t.text "message" + t.datetime "created_at" + t.datetime "updated_at" + t.index ["name"], :name => "index_failed_jobs_on_name", :order => {"name" => :asc} + end + create_table "versions", force: true do |t| t.integer "component_id" t.string "string"