From cbfa33cedc334abe48e8cb49719c0edd106cd0d4 Mon Sep 17 00:00:00 2001 From: Brandon Hilkert Date: Mon, 26 Nov 2012 11:22:48 -0500 Subject: [PATCH] Manage the retrying/deleting of jobs in the Web UI through the 'jid' rather than 'score' so as to avoid accidentally performing actions on multiple jobs when only one was intended. --- lib/sidekiq/api.rb | 17 ++++- lib/sidekiq/web.rb | 97 ++++++++++-------------- myapp/app/controllers/work_controller.rb | 4 +- test/test_api.rb | 14 +++- test/test_web.rb | 54 +++++++------ web/views/retries.slim | 8 +- web/views/retry.slim | 89 +++++++++++----------- web/views/scheduled.slim | 4 +- 8 files changed, 149 insertions(+), 138 deletions(-) diff --git a/lib/sidekiq/api.rb b/lib/sidekiq/api.rb index 58dbd8d8..a6fed517 100644 --- a/lib/sidekiq/api.rb +++ b/lib/sidekiq/api.rb @@ -101,11 +101,24 @@ module Sidekiq end def at - Time.at(@score) + Time.at(score) end def delete - @parent.delete(@score) + @parent.delete(score) + end + + def retry + raise "Retry not available on jobs not in the Retry queue." unless item["failed_at"] + Sidekiq.redis do |conn| + results = conn.zrangebyscore('retry', score, score) + conn.zremrangebyscore('retry', score, score) + results.map do |message| + msg = Sidekiq.load_json(message) + msg['retry_count'] = msg['retry_count'] - 1 + conn.rpush("queue:#{msg['queue']}", Sidekiq.dump_json(msg)) + end + end end end diff --git a/lib/sidekiq/web.rb b/lib/sidekiq/web.rb index 918df216..467fa873 100644 --- a/lib/sidekiq/web.rb +++ b/lib/sidekiq/web.rb @@ -150,14 +150,6 @@ module Sidekiq redirect "#{root_path}queues/#{params[:name]}" end - get "/retries/:score" do - halt 404 unless params[:score] - @score = params[:score].to_f - @retries = retries_with_score(@score) - redirect "#{root_path}retries" if @retries.empty? - slim :retry - end - get '/retries' do @count = (params[:count] || 25).to_i (@current_page, @total_size, @retries) = page("retry", params[:page], @count) @@ -165,6 +157,43 @@ module Sidekiq slim :retries end + get "/retries/:jid" do + halt 404 unless params[:jid] + @retry = Sidekiq::RetrySet.new.select do |retri| + retri.jid == params[:jid] + end.first + redirect "#{root_path}retries" if @retry.nil? + slim :retry + end + + post '/retries' do + halt 404 unless params['jid'] + if params['delete'] + Sidekiq::RetrySet.new.select do |job| + job.jid.in?(params['jid']) + end.map(&:delete) + elsif params['retry'] + Sidekiq::RetrySet.new.select do |job| + job.jid.in?(params['jid']) + end.map(&:retry) + end + redirect "#{root_path}retries" + end + + post "/retries/:jid" do + halt 404 unless params['jid'] + if params['retry'] + Sidekiq::RetrySet.new.select do |job| + job.jid == params['jid'] + end.first.retry + elsif params['delete'] + Sidekiq::RetrySet.new.select do |job| + job.jid == params['jid'] + end.first.delete + end + redirect "#{root_path}retries" + end + get '/scheduled' do @count = (params[:count] || 25).to_i (@current_page, @total_size, @scheduled) = page("schedule", params[:page], @count) @@ -173,58 +202,14 @@ module Sidekiq end post '/scheduled' do - halt 404 unless params[:score] + halt 404 unless params['jid'] halt 404 unless params['delete'] - params[:score].each do |score| - s = score.to_f - process_score('schedule', s, :delete) - end + Sidekiq::ScheduledSet.new.select do |job| + job.jid.in?(params['jid']) + end.map(&:delete) redirect "#{root_path}scheduled" end - post '/retries' do - halt 404 unless params[:score] - params[:score].each do |score| - s = score.to_f - if params['retry'] - process_score('retry', s, :retry) - elsif params['delete'] - process_score('retry', s, :delete) - end - end - redirect "#{root_path}retries" - end - - post "/retries/:score" do - halt 404 unless params[:score] - score = params[:score].to_f - if params['retry'] - process_score('retry', score, :retry) - elsif params['delete'] - process_score('retry', score, :delete) - end - redirect "#{root_path}retries" - end - - def process_score(set, score, operation) - case operation - when :retry - Sidekiq.redis do |conn| - results = conn.zrangebyscore(set, score, score) - conn.zremrangebyscore(set, score, score) - results.map do |message| - msg = Sidekiq.load_json(message) - msg['retry_count'] = msg['retry_count'] - 1 - conn.rpush("queue:#{msg['queue']}", Sidekiq.dump_json(msg)) - end - end - when :delete - Sidekiq.redis do |conn| - conn.zremrangebyscore(set, score, score) - end - end - end - def self.tabs @tabs ||= { "Workers" =>'', diff --git a/myapp/app/controllers/work_controller.rb b/myapp/app/controllers/work_controller.rb index b0854ae0..cc087bac 100644 --- a/myapp/app/controllers/work_controller.rb +++ b/myapp/app/controllers/work_controller.rb @@ -9,7 +9,7 @@ class WorkController < ApplicationController def email UserMailer.delay_for(30.seconds).greetings(Time.now) - render :nothing => true + render :text => 'enqueued' end def long @@ -33,6 +33,6 @@ class WorkController < ApplicationController p2 = Post.second end p.delay.long_method(p2) - render :nothing => true + render :text => 'enqueued' end end diff --git a/test/test_api.rb b/test/test_api.rb index b24ac133..4e5b09cb 100644 --- a/test/test_api.rb +++ b/test/test_api.rb @@ -64,9 +64,21 @@ class TestApi < MiniTest::Unit::TestCase assert_equal 0, r.size end + it 'can retry a retry' do + add_retry + r = Sidekiq::RetrySet.new + assert_equal 1, r.size + r.first.retry + assert_equal 0, r.size + assert_equal 1, Sidekiq::Queue.new('default').size + job = Sidekiq::Queue.new('default').first + assert_equal 'bob', job.jid + assert_equal 1, job['retry_count'] + end + def add_retry at = Time.now.to_f - payload = Sidekiq.dump_json('class' => 'ApiWorker', 'args' => [1, 'mike'], 'queue' => 'default', 'jid' => 'bob') + payload = Sidekiq.dump_json('class' => 'ApiWorker', 'args' => [1, 'mike'], 'queue' => 'default', 'jid' => 'bob', 'retry_count' => 2, 'failed_at' => Time.now.utc) Sidekiq.redis do |conn| conn.zadd('retry', at.to_s, payload) end diff --git a/test/test_web.rb b/test/test_web.rb index 78656d23..00fc1886 100644 --- a/test/test_web.rb +++ b/test/test_web.rb @@ -49,7 +49,7 @@ class TestWeb < MiniTest::Unit::TestCase end it 'handles missing retry' do - get '/retries/12391982.123' + get '/retries/2c4c17969825a384a92f023b' assert_equal 302, last_response.status end @@ -107,17 +107,6 @@ class TestWeb < MiniTest::Unit::TestCase assert_match /HardWorker/, last_response.body end - it 'can delete scheduled' do - msg,score = add_scheduled - Sidekiq.redis do |conn| - assert_equal 1, conn.zcard('schedule') - post '/scheduled', 'score' => [score], 'delete' => 'Delete' - assert_equal 302, last_response.status - assert_equal 'http://example.org/scheduled', last_response.header['Location'] - assert_equal 0, conn.zcard('schedule') - end - end - it 'can display retries' do get '/retries' assert_equal 200, last_response.status @@ -133,31 +122,28 @@ class TestWeb < MiniTest::Unit::TestCase end it 'can display a single retry' do - get '/retries/12938712.123333' + get '/retries/2c4c17969825a384a92f023b' assert_equal 302, last_response.status - _, score = add_retry - - get "/retries/#{score}" + msg = add_retry + get "/retries/#{msg['jid']}" assert_equal 200, last_response.status assert_match /HardWorker/, last_response.body end it 'can delete a single retry' do - _, score = add_retry - - post "/retries/#{score}", 'delete' => 'Delete' + msg = add_retry + post "/retries/#{msg['jid']}", 'delete' => 'Delete' assert_equal 302, last_response.status assert_equal 'http://example.org/retries', last_response.header['Location'] get "/retries" assert_equal 200, last_response.status - refute_match /#{score}/, last_response.body + refute_match /#{msg['args'][2]}/, last_response.body end it 'can retry a single retry now' do - msg, score = add_retry - - post "/retries/#{score}", 'retry' => 'Retry' + msg = add_retry + post "/retries/#{msg['jid']}", 'retry' => 'Retry' assert_equal 302, last_response.status assert_equal 'http://example.org/retries', last_response.header['Location'] @@ -166,6 +152,17 @@ class TestWeb < MiniTest::Unit::TestCase assert_match /#{msg['args'][2]}/, last_response.body end + it 'can delete scheduled' do + msg = add_scheduled + Sidekiq.redis do |conn| + assert_equal 1, conn.zcard('schedule') + post '/scheduled', 'jid' => [msg['jid']], 'delete' => 'Delete' + assert_equal 302, last_response.status + assert_equal 'http://example.org/scheduled', last_response.header['Location'] + assert_equal 0, conn.zcard('schedule') + end + end + it 'can show user defined tab' do begin Sidekiq::Web.tabs['Custom Tab'] = '/custom' @@ -179,14 +176,14 @@ class TestWeb < MiniTest::Unit::TestCase end def add_scheduled + score = Time.now.to_f msg = { 'class' => 'HardWorker', 'args' => ['bob', 1, Time.now.to_f], - 'at' => Time.now.to_f } - score = Time.now.to_f + 'at' => score } Sidekiq.redis do |conn| conn.zadd('schedule', score, Sidekiq.dump_json(msg)) end - [msg, score] + msg end def add_retry @@ -196,12 +193,13 @@ class TestWeb < MiniTest::Unit::TestCase 'error_message' => 'Some fake message', 'error_class' => 'RuntimeError', 'retry_count' => 0, - 'failed_at' => Time.now.utc, } + 'failed_at' => Time.now.utc, + 'jid' => "f39af2a05e8f4b24dbc0f1e4"} score = Time.now.to_f Sidekiq.redis do |conn| conn.zadd('retry', score, Sidekiq.dump_json(msg)) end - [msg, score] + msg end end end diff --git a/web/views/retries.slim b/web/views/retries.slim index 75985b79..dbcab859 100755 --- a/web/views/retries.slim +++ b/web/views/retries.slim @@ -17,18 +17,18 @@ header.row th Queue th Worker th Args - - @retries.each do |(msg, score)| + - @retries.each do |msg, score| tr td - input type='checkbox' name='score[]' value='#{score}' + input type='checkbox' name='jid[]' value='#{msg['jid']}' td - a href="#{root_path}retries/#{score}"== relative_time(Time.at(score)) + a href="#{root_path}retries/#{msg['jid']}"== relative_time(Time.at(score)) td= msg['retry_count'] td a href="#{root_path}queues/#{msg['queue']}" #{msg['queue']} td= msg['class'] td= display_args(msg['args']) input.btn.btn-danger.btn-small.pull-right type="submit" name="delete" value="Delete" - input.btn.btn-primary.btn-small.pull-right type="submit" name="retry" value="Retry Now" + input.btn.btn-primary.btn-small.pull-right type="submit" name="retry" value="Retry Now" - else .alert.alert-success No retries were found diff --git a/web/views/retry.slim b/web/views/retry.slim index 4acec003..5f7670ef 100755 --- a/web/views/retry.slim +++ b/web/views/retry.slim @@ -1,52 +1,55 @@ header h3 Job -- @retries.each do |msg| - table class="retry table table-bordered table-striped" - tbody +table class="retry table table-bordered table-striped" + tbody + tr + th Queue + td + a href="#{root_path}queues/#{@retry['queue']}" #{@retry['queue']} + tr + th Job Class + td + code= @retry['class'] + tr + th Job Arguments + td + code= display_args(@retry['args'], 1000) + tr + th Job ID + td + code= @retry.jid + - if @retry['retry_count'] > 0 tr - th Queue - td - a href="#{root_path}queues/#{msg['queue']}" #{msg['queue']} + th Retry Count + td= @retry['retry_count'] tr - th Job Class - td - code= msg['class'] + th Last Retry + td== relative_time(@retry['retried_at'].is_a?(Numeric) ? Time.at(@retry['retried_at']) : Time.parse(@retry['retried_at'])) + - else tr - th Job Arguments - td - code= display_args(msg['args'], 1000) - - if msg['retry_count'] > 0 - tr - th Retry Count - td= msg['retry_count'] - tr - th Last Retry - td== relative_time(msg['retried_at'].is_a?(Numeric) ? Time.at(msg['retried_at']) : Time.parse(msg['retried_at'])) - - else - tr - th Originally Failed - td== relative_time(msg['failed_at'].is_a?(Numeric) ? Time.at(msg['failed_at']) : Time.parse(msg['failed_at'])) - tr - th Next Retry - td== relative_time(Time.at(@score)) + th Originally Failed + td== relative_time(@retry['failed_at'].is_a?(Numeric) ? Time.at(@retry['failed_at']) : Time.parse(@retry['failed_at'])) + tr + th Next Retry + td== relative_time(Time.at(@retry.score)) - h3 Error - table class="error table table-bordered table-striped" - tbody +h3 Error +table class="error table table-bordered table-striped" + tbody + tr + th Error Class + td + code= @retry['error_class'] + tr + th Error Message + td= @retry['error_message'] + - if !@retry['error_backtrace'].nil? tr - th Error Class + th Error Backtrace td - code= msg['error_class'] - tr - th Error Message - td= msg['error_message'] - - if !msg['error_backtrace'].nil? - tr - th Error Backtrace - td - code== msg['error_backtrace'].join("
") - form.form-horizontal action="#{root_path}retries/#{@score}" method="post" - a.btn href="#{root_path}retries" ← Back - input.btn.btn-primary type="submit" name="retry" value="Retry Now" - input.btn.btn-danger type="submit" name="delete" value="Delete" + code== @retry['error_backtrace'].join("
") +form.form-horizontal action="#{root_path}retries/#{@retry.jid}" method="post" + a.btn href="#{root_path}retries" ← Back + input.btn.btn-primary type="submit" name="retry" value="Retry Now" + input.btn.btn-danger type="submit" name="delete" value="Delete" diff --git a/web/views/scheduled.slim b/web/views/scheduled.slim index 9b93d3fc..68be56ae 100755 --- a/web/views/scheduled.slim +++ b/web/views/scheduled.slim @@ -16,10 +16,10 @@ header.row th width="10%" Queue th Worker th Args - - @scheduled.each do |(msg, score)| + - @scheduled.each do |msg, score| tr td - input type='checkbox' name='score[]' value='#{score}' + input type='checkbox' name='jid[]' value='#{msg['jid']}' td== relative_time(Time.at(score)) td a href="#{root_path}queues/#{msg['queue']}" #{msg['queue']}